-
Notifications
You must be signed in to change notification settings - Fork 658
Update mobile and desktop toolbars #10407
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
base: main
Are you sure you want to change the base?
Update mobile and desktop toolbars #10407
Conversation
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.
Pull Request Overview
This PR adds an accessible visible label for the duration selector in the Metrics page by generating a unique ID and linking a FluentInputLabel
to the FluentSelect
, improving a11y.
- Generate a private readonly
_selectDurationId
in the code-behind. - Insert a
FluentInputLabel
withForId
and matchingId
on the select. - Apply inline margin styling to space the label consistently.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Aspire.Dashboard/Components/Pages/Metrics.razor.cs | Adds _selectDurationId field to hold a unique ID for the select |
src/Aspire.Dashboard/Components/Pages/Metrics.razor | Adds FluentInputLabel linked via ForId and sets Id on FluentSelect ; applies inline margin |
Comments suppressed due to low confidence (1)
src/Aspire.Dashboard/Components/Pages/Metrics.razor:62
- Add a component or unit test to verify that the label is correctly associated with the select via the generated
Id
andForId
, ensuring the accessibility improvement is covered by tests.
<FluentInputLabel slot="end"
Label="@Loc[nameof(Dashboard.Resources.Metrics.MetricsSelectADuration)]" | ||
ForId="@_selectDurationId" | ||
Orientation="Orientation.Horizontal" | ||
style="margin-right: calc(var(--toolbar-item-gap) * 2)" /> |
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.
[nitpick] Consider extracting the inline calc(var(--toolbar-item-gap) * 2)
into a CSS class or CSS variable instead of using a magic number multiplier in the style attribute to improve maintainability.
Copilot uses AI. Check for mistakes.
<FluentInputLabel slot="end" | ||
Label="@Loc[nameof(Dashboard.Resources.Metrics.MetricsSelectADuration)]" | ||
ForId="@_selectDurationId" | ||
Orientation="Orientation.Horizontal" | ||
style="margin-right: calc(var(--toolbar-item-gap) * 2)" /> |
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.
On the structured logs page a divider is used to space them out:
aspire/src/Aspire.Dashboard/Components/Pages/StructuredLogs.razor
Lines 53 to 71 in 37d5759
@if (ViewportInformation.IsDesktop) | |
{ | |
<div title="@Loc[nameof(Dashboard.Resources.StructuredLogs.StructuredLogsMinimumLogFilter)]" slot="end" style="display: flex;align-items: center;"> | |
<span>@Loc[nameof(Dashboard.Resources.StructuredLogs.StructuredLogsLevels)]</span> | |
<FluentDivider Role="DividerRole.Presentation" Orientation="Orientation.Vertical" /> | |
<LogLevelSelect IncludeLabel="false" | |
@bind-LogLevel="@PageViewModel.SelectedLogLevel" | |
LogLevels="@_logLevels" | |
HandleSelectedLogLevelChangedAsync="@HandleSelectedLogLevelChangedAsync" /> | |
</div> | |
} | |
else | |
{ | |
<LogLevelSelect IncludeLabel="true" | |
@bind-LogLevel="@PageViewModel.SelectedLogLevel" | |
LogLevels="@_logLevels" | |
HandleSelectedLogLevelChangedAsync="@HandleSelectedLogLevelChangedAsync" /> | |
} |
I think we should do that to be consistent.
Also, how does this look when the page is small and the UI is in a dialog. Should it follow what structured logs page does?
Finally, I see the structured logs page has a simple span instead of a label. Could you update that page to use a label as well?
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.
Made the metrics toolbar consistent and removed spans. I also went through the rest of the pages because some mobile views were outdated or inconsistent and cleaned them up.
I disagree for consistency. We don't put data on the mobile settings page anywhere else. |
@JamesNK moved it back |
Same thing on traces page. Data doesn't immediately update after adding/removing a filter. |
Trace detail: Check what happens when there is a copilot button visible. Same with structured logs (when there is an error). Is it displayed in appropriate places in desktop and mobile? |
|
||
@code { |
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 add a code behind file. C# is better supported in C# files.
</FluentStack> | ||
} | ||
|
||
@code { |
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 add a code behind file. C# is better supported in C# files.
<FluentDivider slot="end" Role="DividerRole.Presentation" Orientation="Orientation.Vertical" /> | ||
} | ||
|
||
@code { |
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 add a code behind file. C# is better supported in C# files.
<div class="trace-header-filters"> | ||
@if (ViewportInformation.IsDesktop) | ||
{ | ||
<div slot="" class="trace-header"> |
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'd rather this trace header also be visible on the page in mobile view. Even though it is displayed in the header, and we typically don't display header information, this summary of the trace is important don't.
Don't display it in the mobile filter popup
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.
The overflow should prevent it from taking up a lot of room on small screens. It will be one like max.
<ul> | ||
<li>@Loc[nameof(Dashboard.Resources.TraceDetail.TraceDetailTraceStartHeader)] <strong | ||
title="@FormatHelpers.FormatDateTime(TimeProvider, _trace.FirstSpan.StartTime, MillisecondsDisplay.Full)">@FormatHelpers.FormatDateTime(TimeProvider, _trace.FirstSpan.StartTime, MillisecondsDisplay.Truncated)</strong> | ||
</li> | ||
<li>@Loc[nameof(Dashboard.Resources.TraceDetail.TraceDetailDurationHeader)] <strong>@DurationFormatter.FormatDuration(trace.Duration)</strong> | ||
</li> | ||
<li>@Loc[nameof(Dashboard.Resources.TraceDetail.TraceDetailResourcesHeader)] <strong>@_resourceCount</strong> | ||
</li> | ||
<li>@Loc[nameof(Dashboard.Resources.TraceDetail.TraceDetailDepthHeader)] <strong>@_maxDepth</strong> | ||
</li> | ||
<li>@Loc[nameof(Dashboard.Resources.TraceDetail.TraceDetailTotalSpansHeader)] <strong>@trace.Spans.Count</strong> | ||
</li> | ||
</ul> |
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.
Remove
Description
Adds duration text to the metrics page (fixes #10159), adds the clock back to desktop view, and improves mobile view for all pages.
Resources
Before/after (mobile)
Before/after (desktop)
Console logs
Before/after (mobile)
Before/after (desktop)
Structured logs
Before/after (mobile)
Before/after (desktop)
Traces
Before/after (mobile)
Before/after (desktop)
Trace details
Before/after (mobile)
(I think it's more useful on trace details to put this information directly on the page, but it's not important - happy to revert that change if you disagree)
Before/after (desktop)
Metrics
Before/after (mobile)
Before/after (desktop)
Checklist
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template