-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[dashpages] Dashpage layout improvements #5784
base: feature/dashpages
Are you sure you want to change the base?
[dashpages] Dashpage layout improvements #5784
Conversation
`FluentGrid` uses `@media` queries for breakpoints, which work on the window size (viewport area). However we have a splitter in this UI, and dragging it should cause the dashpage area to relayout as the container size changes. This change removes `FluentGrid` and instead uses flexbox layout directly.
@@ -10,44 +10,27 @@ | |||
<h3 title="@SelectedDashpage.Name">@SelectedDashpage.Name</h3> | |||
} | |||
|
|||
<FluentGrid Spacing="3" AdaptiveRendering="true" Justify="JustifyContent.FlexStart" Class="dashpage-chart-grid"> | |||
<section class="dashpage-chart-grid"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a section element instead of div?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section
gives a bit more semantics, suggesting it's a larger area of content. Similarly, article
is used for each chart. This theoretically gives tools that inspect the DOM (such as screen readers) more info about the DOM. div
s get used for non-semantic layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry about being semantically correct. We're using div everywhere for block content and span for inline content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is preferred to use the semantically correct element. I would not change to div just for the sake of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't using semantically correct elements in other places. It seems like a non-goal and makes it different from everywhere else in the app which is always to use div. Often being consistently correct is better than being technically correct.
But arguing over html elements is the least productive thing in the world so if you think this is best then go ahead.
@foreach (var chart in SelectedDashpage.Charts) | ||
{ | ||
if (Instruments.FirstOrDefault(i => i.Name == chart.InstrumentName) is { } instrument) | ||
if (Instruments.FirstOrDefault(i => StringComparers.OtlpInstrumentName.Equals(i.Name, chart.InstrumentName)) is { } instrument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: string.Equals + StringComparison.OtlpInstrumentName is more idomatic in this sitation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems subjective. I prefer this form as it's less code and feels more natural to me to use the right object for the operation, than passing an enum to a method to modify its behaviour. I feel like StringComparison
only exists for historical reasons, and could be removed from the library if it weren't for backcompat requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringComparer is for non-string only, generic scenarios, e.g. the key comparer in a dictionary, or sorting in LINQ:
https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#methods-that-perform-string-comparison-indirectly
StringComparison is for string operations:
https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#recommendations-for-string-usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SO answer summerizies it well: https://stackoverflow.com/a/8918112
I feel like we've had this conversation before.
} | ||
|
||
.dashpage-chart-grid .dashpage-chart-tile { | ||
flex: 1 1 25%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a small comment here that says what this does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what's unclear? Do you want me to explain how flex
works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what 1 1 25%
means. I'm more experienced with css grid than flex box and I'm guessing most people who see this won't know what it does.
Make dashpage responsive to container, not viewport
FluentGrid
uses@media
queries for breakpoints, which work on the window size (viewport area). However we have a splitter in this UI, and dragging it should cause the dashpage area to relayout as the container size changes.This change removes
FluentGrid
and instead uses flexbox layout directly.Note
This PR is to the
feature/dashpages
branch, notmain
.Before
After
Microsoft Reviewers: Open in CodeFlow