Dashboard: design-token system, typography refresh, UX polish & axe accessibility tests - #18943
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18943Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18943" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Not ready to approve
Accessibility regressions, existing test failures, and premature axe scans must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Introduces a centralized Dashboard design system, refreshes navigation and typography, polishes interactions, and adds automated accessibility coverage.
Changes:
- Adds design tokens, self-hosted fonts, theme updates, and responsive UI styling.
- Adds collapsible navigation, grid/scroll enhancements, and accessibility fixes.
- Adds axe-core Playwright tests and localized accessibility labels.
File summaries
| File | Description |
|---|---|
Directory.Packages.props |
Adds axe-core package version. |
tests/Aspire.Dashboard.Tests/Aspire.Dashboard.Tests.csproj |
References axe-core Playwright. |
tests/Aspire.Dashboard.Tests/Integration/Playwright/AccessibilityTests.cs |
Adds accessibility scans. |
src/Aspire.Dashboard/wwwroot/css/tokens.css |
Defines design tokens and fonts. |
src/Aspire.Dashboard/wwwroot/css/app.css |
Applies broad visual and UX changes. |
src/Aspire.Dashboard/wwwroot/css/markdown.css |
Uses the mono font token. |
src/Aspire.Dashboard/wwwroot/js/app.js |
Adds grid, scroll, and hover interactions. |
src/Aspire.Dashboard/wwwroot/js/app-theme.js |
Connects tokens to Fluent theming. |
src/Aspire.Dashboard/wwwroot/fonts/geist/LICENSE.txt |
Adds Geist license. |
src/Aspire.Dashboard/wwwroot/fonts/geist/Geist-Variable.woff2 |
Adds Geist variable font. |
src/Aspire.Dashboard/wwwroot/fonts/geist/GeistMono-Variable.woff2 |
Adds Geist Mono font. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/LICENSE.txt |
Adds Poppins license. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/latin-500-normal.woff2 |
Adds Poppins Latin 500. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/latin-600-normal.woff2 |
Adds Poppins Latin 600. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/latin-700-normal.woff2 |
Adds Poppins Latin 700. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/latin-ext-500-normal.woff2 |
Adds extended Latin 500. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/latin-ext-600-normal.woff2 |
Adds extended Latin 600. |
src/Aspire.Dashboard/wwwroot/fonts/poppins/latin-ext-700-normal.woff2 |
Adds extended Latin 700. |
src/Aspire.Dashboard/Utils/DashboardUIHelpers.cs |
Defines the page-title section key. |
src/Aspire.Dashboard/Utils/BrowserStorageKeys.cs |
Adds persisted navigation state key. |
src/Aspire.Dashboard/Components/App.razor |
Loads tokens and preloads fonts. |
src/Aspire.Dashboard/Components/_Imports.razor |
Imports Blazor sections. |
src/Aspire.Dashboard/Components/Layout/MainLayout.razor |
Adds full-width header and nav toggle. |
src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs |
Persists navigation expansion state. |
src/Aspire.Dashboard/Components/Layout/MainLayout.razor.css |
Restyles desktop layout and rail. |
src/Aspire.Dashboard/Components/Layout/DesktopNavMenu.razor |
Renames structured-logs navigation. |
src/Aspire.Dashboard/Components/Layout/MobileNavMenu.razor.cs |
Updates the mobile navigation label. |
src/Aspire.Dashboard/Components/Layout/AspirePageContentLayout.razor |
Moves page titles into the header. |
src/Aspire.Dashboard/Components/Layout/AspirePageContentLayout.razor.cs |
Adds toolbar hiding support. |
src/Aspire.Dashboard/Components/Pages/Resources.razor |
Combines tabs and desktop filters. |
src/Aspire.Dashboard/Components/Pages/Resources.razor.css |
Styles the combined toolbar row. |
src/Aspire.Dashboard/Components/ResourcesGridColumns/ResourceNameDisplay.razor |
Enlarges resource icons. |
src/Aspire.Dashboard/Components/Controls/ResourceActions.razor |
Aligns action-button slots. |
src/Aspire.Dashboard/Components/Controls/ResourceActions.razor.css |
Styles fixed-width action slots. |
src/Aspire.Dashboard/Components/Controls/AspireMenuButton.razor |
Changes menu ARIA metadata. |
src/Aspire.Dashboard/Components/Controls/TextVisualizer.razor |
Names and focuses its scroll region. |
src/Aspire.Dashboard/Components/Controls/TextVisualizer.razor.cs |
Injects localized control strings. |
src/Aspire.Dashboard/Components/Controls/SummaryDetailsView.razor.css |
Retints detail headers. |
src/Aspire.Dashboard/Components/Dialogs/SettingsDialog.razor |
Names the language selector. |
src/Aspire.Dashboard/Components/Dialogs/TextVisualizerDialog.razor |
Promotes the copy action. |
src/Aspire.Dashboard/Components/Dialogs/FilterDialog.razor |
Uses accent styling for Apply. |
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor |
Changes secret-toggle focus behavior. |
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor.cs |
Suppresses native reveal controls. |
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor.js |
Injects password-control styling. |
src/Aspire.Dashboard/Resources/Layout.resx |
Adds navigation-toggle strings. |
src/Aspire.Dashboard/Resources/Layout.Designer.cs |
Exposes generated layout strings. |
src/Aspire.Dashboard/Resources/ControlsStrings.resx |
Adds the text-region label. |
src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs |
Exposes the generated label. |
src/Aspire.Dashboard/Resources/xlf/Layout.cs.xlf |
Syncs Czech layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.de.xlf |
Syncs German layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.es.xlf |
Syncs Spanish layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.fr.xlf |
Syncs French layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.it.xlf |
Syncs Italian layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.ja.xlf |
Syncs Japanese layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.ko.xlf |
Syncs Korean layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.pl.xlf |
Syncs Polish layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.pt-BR.xlf |
Syncs Portuguese layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.ru.xlf |
Syncs Russian layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.tr.xlf |
Syncs Turkish layout resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.zh-Hans.xlf |
Syncs simplified Chinese resources. |
src/Aspire.Dashboard/Resources/xlf/Layout.zh-Hant.xlf |
Syncs traditional Chinese resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.cs.xlf |
Syncs Czech control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.de.xlf |
Syncs German control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.es.xlf |
Syncs Spanish control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.fr.xlf |
Syncs French control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.it.xlf |
Syncs Italian control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.ja.xlf |
Syncs Japanese control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.ko.xlf |
Syncs Korean control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.pl.xlf |
Syncs Polish control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.pt-BR.xlf |
Syncs Portuguese control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.ru.xlf |
Syncs Russian control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.tr.xlf |
Syncs Turkish control resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.zh-Hans.xlf |
Syncs simplified Chinese resources. |
src/Aspire.Dashboard/Resources/xlf/ControlsStrings.zh-Hant.xlf |
Syncs traditional Chinese resources. |
Review details
Files not reviewed (2)
- src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs: Generated file
- src/Aspire.Dashboard/Resources/Layout.Designer.cs: Generated file
- Files reviewed: 64/74 changed files
- Comments generated: 7
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
PR Testing Report — #18943PR Information
Artifact / Version Verification
Changes AnalyzedDashboard-only change (74 files, +3292/-176). No public API changes.
Test Scenarios ExecutedScenario 1 — Build validationObjective: Confirm the PR's "Dashboard + Aspire.Dashboard.Tests build clean (0 warnings / 0 errors)" claim.
Scenario 2 — Accessibility suite (headline)Objective: Verify the PR's "19/19 passing" claim for the new The suite is 19 cases: 10 (5 pages × light/dark) + 3 (home @ mobile/tablet/desktop) + 4 (Settings/Filter dialogs × light/dark) + 2 (codeblock contrast). All are
The one failure (2 serious/critical violations on Why this is flaky, not a real page regression: the separate Root cause: Impact: intermittent red in the Outerloop workflow. This is the well-known "readiness check / race condition on startup" flaky pattern. Suggested fix: before running axe, also wait for grid content to be present and loading spinners ( Scenario 3 — Visual evidence captureObjective: Confirm the design-token / typography / nav / toolbar changes render correctly. Captured via a temporary Playwright test against the same mock-data dashboard fixture (8 screenshots, light+dark, desktop→mobile). Observations:
Unhappy-Path / Boundary Coverage
Summary
Overall Result✅ Verified, with one issue found. The build is clean, the visual/UX changes render correctly across themes and viewports, and nav-collapse persistence works. The one issue is test robustness, not product behavior: the new RecommendationStrengthen the readiness gate in Tested from an isolated worktree at the PR head; screenshots captured locally via a throwaway Playwright test that was not added to the PR. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Thanks for the review — all four findings are addressed in aa4e323, plus a reliability pass on the new a11y tests: 1. (Medium) Functional tests for the 3 net-new JS behaviors — Added
2. (Medium) Hardcoded English scroll-button labels — The labels are now localized in .NET (new 3. (Low) 4. (Low) Body-wide subtree MutationObserver — Kept Plus — a11y test flakiness: |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Not ready to approve
Accessibility regressions, stale menu-state tests, and dynamic scroll-region update handling must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs: Generated file
- src/Aspire.Dashboard/Resources/Layout.Designer.cs: Generated file
Comments suppressed due to low confidence (5)
src/Aspire.Dashboard/Components/Controls/AspireMenuButton.razor:14
- Removing
aria-expandedmeans the menu button no longer exposes whether its popup is open, so screen-reader users lose the state required by the menu-button pattern. It also guarantees failures in the existingResourcesTests.ViewOptionsMenu_ReportsExpandedStateandAspireMenuButtonFocusTestsPlaywright tests, which assert the false/true/false transitions. Please forward the state to the actual shadow-DOM button (or otherwise expose it on an element with the button role) and update the tests to target that valid element.
// opens a menu. We intentionally do NOT set aria-expanded here: aria-expanded is only permitted on
// a specific set of roles (button, combobox, etc.), and the <fluent-button> host is generic, so
// axe/Accessibility Insights flag it as WCAG 4.1.2 aria-allowed-attr. Moving role="button" onto the
// host would make aria-expanded legal but introduces a nested-interactive violation because Fluent's
// shadow DOM already renders a real <button>. Relying on aria-haspopup alone is the accessible,
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor:76
- This removes the only keyboard-reachable control for showing or hiding the secret. The native reveal control is now suppressed, while this replacement button is excluded from sequential focus, so keyboard-only users cannot invoke the action. Keep the custom button in the tab order.
tabindex="-1"
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor.cs:151
- Setting
_pendingHideNativeRevealdoes not schedule another render. IfhideNativeRevealreturnsfalsein the not-yet-upgraded case described above,OnAfterRenderAsyncwill not retry until some unrelated UI event happens, leaving the duplicate native reveal control visible. Wait for the custom element/shadow root in JS or schedule a bounded retry explicitly.
_pendingHideNativeReveal = !allApplied;
src/Aspire.Dashboard/wwwroot/js/app.js:762
- Ignoring content-only mutations prevents these controls from activating when a registered region grows after registration. For example, a console log region can start below the 240px threshold and then accumulate rows; its box size stays fixed, so
ResizeObserverdoes not fire, and no update occurs until the user scrolls or resizes the window. Schedule an rAF-throttled visibility update for mutations inside registered containers while keeping full rescans limited to structural triggers.
src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs:438 - The new collapse/expand behavior and its local-storage restoration have no focused browser test. A regression in the dynamic label/icon, rail class, or persisted state would not be caught by the axe scans. Add Playwright coverage that toggles the rail, verifies the collapsed/expanded UI, reloads, and confirms the saved state is restored.
private async Task ToggleNavMenuExpandedAsync()
{
_isNavMenuExpanded = !_isNavMenuExpanded;
await LocalStorage.SetUnprotectedAsync(BrowserStorageKeys.NavMenuExpanded, _isNavMenuExpanded);
}
- Files reviewed: 65/75 changed files
- Comments generated: 0 new
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
|
Gap to the right of tables on pages. I believe this is common to all content displayed and isn't specific to grids: I don't like highlighting child with the parent. Also the child button background color is wrong. Return to the old behavior for now: Content has too many different positions in details view. Note there were two positions before: Scrollbars are too light. Need to be more subtle: nit: See whether these can line up. It wasn't good before: Remove Collapse sidebar text: The no data message on grids should have padding above/below the message: Trace detail page needs a lot of work. This grid had a lot of customization in its CSS. There shouldn't be horizontal lines between rows, and the vertical time markers should be continious lines: Header needs to gracefully handle a long title (i.e. cut off content with ellipsis): Colors used for code blocks need work. Doesn't look good in light or dark: I'm not sure I like the scroll to top/bottom buttons always being visible when not at the top or bottom. They're a little over bearing. Maybe only appear when the mouse is near the area? What do other websites do? |
There was a problem hiding this comment.
🟡 Not ready to approve
Keyboard-accessibility gaps and scroll/readiness bugs can leave features unavailable or tests false-passing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs: Generated file
- src/Aspire.Dashboard/Resources/Layout.Designer.cs: Generated file
Suppressed comments (6)
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor:76
- Removing this show/hide button from the tab order makes the secret-value visibility action mouse-only. The same change also suppresses the browser's native reveal control, so keyboard users have no remaining way to perform this action. Leave the custom button keyboard-focusable.
tabindex="-1"
tests/Aspire.Dashboard.Tests/Integration/Playwright/AccessibilityTests.cs:239
- The page title is rendered before the Resources page finishes its async resource subscription, so this readiness check can let axe scan the loading shell before the fixture's
frontendrow and its controls exist. Waiting for component upgrades/fonts does not wait for that Blazor data update, which makes the new gate capable of false-passing. For the root route, wait for the fixture resource to be visible before scanning.
src/Aspire.Dashboard/wwwroot/js/app.js:773 - Content growth inside an already-registered scroll region never refreshes the controls.
ResizeObserveronly observes the container box, notscrollHeight, and this observer explicitly ignores the log/grid mutations that make an initially short region overflow. As a result, streaming console/structured logs can remain without buttons until some unrelated scroll or resize occurs; the test only covers a region that is already overflowing when inserted. Schedule the inexpensive rAF update for mutations inside registered targets while still reserving full rescans for target/dialog additions.
src/Aspire.Dashboard/wwwroot/js/app.js:600 - These visible buttons are removed from sequential keyboard navigation, but not every target has another keyboard path:
TextVisualizerDialog.razormakes.markdown-contentanoverflow-y:autoscroll container without atabindex. Keyboard users therefore cannot focus that region or invoke either jump action. Keep the buttons tabbable (or make every target focusable with an equivalent accessible control).
src/Aspire.Dashboard/Components/Dialogs/InteractionsInputDialog.razor.js:36 - Returning
falsehere does not actually guarantee a retry:OnAfterRenderAsynconly updates_pendingHideNativeRevealand does not request another render, and Blazor does not rerender whenOnAfterRenderAsynccompletes. If the Fluent element is not upgraded on the first call, the native reveal control can remain indefinitely. Wait for the custom element definition within this JS call instead of relying on an unrelated future render.
export function hideNativeReveal(hostId) {
const host = document.getElementById(hostId);
if (!host || !host.shadowRoot) {
return false;
src/Aspire.Dashboard/Components/Layout/MainLayout.razor:66
- The new navigation interaction and persistence path has no focused coverage: the Playwright interaction tests exercise grid auto-fit, row hover, and scroll controls, while the axe matrix only scans the default collapsed state. Add a browser test that toggles this button, verifies the expanded class/labels, reloads, and confirms the local-storage state is restored; otherwise the central new interaction can break without any test failing.
<FluentButton Class="nav-toggle-button nav-toggle-bottom"
Appearance="Appearance.Stealth" OnClick="@ToggleNavMenuExpandedAsync"
Title="@(_isNavMenuExpanded ? Loc[nameof(Layout.NavMenuToggleCollapse)] : Loc[nameof(Layout.NavMenuToggleExpand)])"
aria-label="@(_isNavMenuExpanded ? Loc[nameof(Layout.NavMenuToggleCollapse)] : Loc[nameof(Layout.NavMenuToggleExpand)])">
- Files reviewed: 66/76 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Line these up: I think the color of information bar could be more subtle. Just a bit lighter than the dark background (like how in light mode the color is slightly darker than the light background): The GenAI dialog needs a lot of work in dark mode. It is fine in light mode. You can generate a genai trace in the stress playground app using stress-apiservice command: Is it intentional that there is different spacing from the horizontal delimiter? I think more spacing before the page title would be good. |
|
Mouseover on the manage logs and telemetry page (via settings) is grey. Also the header is a dark color, which is different than before but I'm not sure is intentional or not. The dropdown button for instrument filters increases in size when it becomes selected: |
|
On the colors, the light mode feels ok. There is a tiny hint of purple. Dark mode feels very purple. Especially dialogs. What do you think of toning dark mode purple down? Maybe get some before and after screenshots for people to compare. I wouldn't mind seeing what the design looks like with completely neutral backgrounds. I think the purple theme would still come through in icons, highlights, scrollbars, etc. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
… theming) Root-caused and fixed all 11 items from the PR #18943 screenshot review: - Resources Actions column -> auto so the action icons hug the content right edge instead of leaving a large dead gap; freed width goes to the URLs column. - Remove the parent-row hover ownership highlight (the app.js feature + CSS rule and its now-stale interaction test + child fixture resource). - Align the property-grid Name/Value headers with their values: the unsortable <th> rendered flush while Fluent inset the <td> 16px, so zero the cell inline-start padding to kill the stair-step. - Quiet, tint-free neutral scrollbars in both themes (was brand violet). - Drop the visible "Collapse sidebar" label; the icon + aria-label carry it. - Keep the empty-state ("No X found") cell padding by excluding it from the compact row-padding rule so the message isn't cramped. - Trace waterfall: re-zero cell padding/border (row-scoped, !important) so the vertical time markers are continuous full-height lines and there are no horizontal row separators. - Ellipsize long page/trace titles in the header (display:block) instead of bleeding into the top-right nav icons. - New --codeblock-header-background-color token so the code block header strip is a subtle step off the body in both light and dark. - Scroll-to-top/bottom buttons reveal on pointer proximity (200ms anti-flicker hide) instead of being always-on; keyboard focus still force-reveals them. - Line up the span/log detail flyout's trailing controls: the header (panel-resize + Close) shipped a 12px inline padding and the filter toolbar fell back to Fluent's default 8px, so the Close and options buttons stair-stepped ~4-6px short of the page toolbar's trailing control. Pin both to the shared page gutter (--layout-right-padding) so they land on one clean vertical edge with the toolbar. Desktop split view only; below 768px the flyout stacks full-width and keeps its own header inset. Validation: dashboard build 0/0; DashboardInteractionsTests 2/2; Accessibility Tests 19/19; test project 0/0. Verified live in light + dark, including the flyout right-edge alignment (Close, filter options, and page-toolbar options all resolve to the same right x). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b530660c-2232-498c-b083-c1d1d6f7227a
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Not ready to approve
Scroll-button activation can become stale, the contrast probe measures the wrong backgrounds, and the auto-fit interaction remains unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs: Generated file
- src/Aspire.Dashboard/Resources/Layout.Designer.cs: Generated file
Suppressed comments (4)
src/Aspire.Dashboard/wwwroot/js/app.js:621
- This observer only watches the scroll viewport's border box. Live log/trace updates change the child size and
scrollHeightwithout resizing that fixed viewport, whileonBodyMutationsexplicitly ignores content churn, sois-activecan remain stale when overflow crosses 240px until another scroll/window resize occurs. Observe the scrolling content or schedule an update for mutations inside registered targets, and make the interaction test grow content after registration.
tests/Aspire.Dashboard.Tests/Integration/Playwright/AccessibilityTests.cs:443 - The probe measures against a synthetic
--neutral-layer-1background, not either production surface it claims to protect. Markdown code blocks use--codeblock-background-color(--neutral-layer-2), while the text visualizer is transparent over the dialog background. A palette can therefore pass here while failing on the rendered background. Render probes in the actual code-block/dialog contexts and measure each effective background.
src/Aspire.Dashboard/Components/App.razor:16 - Preloading Geist Mono forces every dashboard visit to download it, even though the initial Resources page has no code and the monospace stack prefers installed Cascadia Mono. Let the
@font-faceload it lazily when code is rendered; only the body font benefits from an unconditional preload.
<link rel="preload" href="fonts/geist/GeistMono-Variable.woff2" as="font" type="font/woff2" crossorigin />
tests/Aspire.Dashboard.Tests/Integration/Playwright/DashboardInteractionsTests.cs:114
- This describes and purports to test behavior that no longer exists: neither
app.jsnorapp.csstogglesis-hovered, and.scroll-button.is-visibleis visible without pointer proximity. The hover therefore proves nothing and the comment misleadingly narrates earlier review evolution. Assert the current always-visible behavior directly.
- Files reviewed: 79/89 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Not ready to approve
Live-content overflow is not re-evaluated, and the contrast probe measures non-production backgrounds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs: Generated file
- src/Aspire.Dashboard/Resources/Layout.Designer.cs: Generated file
Suppressed comments (3)
src/Aspire.Dashboard/wwwroot/js/app.js:725
- Pure content churn is exactly what increases
scrollHeight, but it is ignored here. AResizeObserverdoes not fire when an overflow container keeps the same box size while log rows are appended, and no scroll event occurs until the user interacts, so a region registered below 240px can remain permanently inactive after live data grows past the threshold. Schedule an update for child-list mutations inside a registered target, and cover the incremental-growth case rather than injecting a region that already overflows.
tests/Aspire.Dashboard.Tests/Integration/Playwright/AccessibilityTests.cs:443 - This probe does not use either production background it claims to guard: markdown code blocks use
--codeblock-background-color(--neutral-layer-2), while the text visualizer is transparent over--dialog-background-color; the probe forces--neutral-layer-1. A palette can therefore pass this calculation while failing on the actual rendered surface, particularly the newly darkened light-theme code block. Render and measure the real markdown and dialog/text-visualizer DOM (or test both computed production backgrounds).
tests/Aspire.Dashboard.Tests/Integration/Playwright/DashboardInteractionsTests.cs:114 - This block describes and tests an
.is-hoveredproximity gate that no longer exists:app.jsnever toggles that class, and the current CSS makes.is-visibleimmediately visible. The hover is therefore a no-op and the comments contradict the behavior under test. Assert visibility directly so the test documents the current always-visible affordance.
- Files reviewed: 79/89 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
257d85f to
6538952
Compare
This comment has been minimized.
This comment has been minimized.
AspireMenuButton put aria-haspopup/aria-expanded on the role-less <fluent-button> host, which axe-core flags as a critical aria-allowed-attr violation (neither is a global ARIA attribute). Replace the per-button OnAfterRender interop and declarative host attributes with a single document-level MutationObserver keyed by a neutral data-aspire-menu-trigger marker. The observer moves both attributes onto the inner native button[part~=control] (which has an implicit role=button): aria-haspopup=menu is set directly, and the aria-expanded that FluentMenu re-stamps on the host is captured, stripped from the host (keeping it axe-clean), and re-pinned onto the inner control. FAST asynchronously mirrors/clears the host's aria-expanded onto the inner button, so the observer watches both targets and re-applies the authoritative value; equality guards prevent observer recursion. The trigger therefore still reports its expanded state to assistive technology while the host carries no disallowed attributes. Also point the Resources context-menu FluentMenu anchor at a hidden, non-focusable <button> so the aria-expanded FluentMenu stamps on its anchor no longer lands on the visible role-less summary <div>. Accessibility axe class passes 19/19 (was 2/19); the menu-button aria-expanded outerloop tests (ViewOptionsMenu_ReportsExpandedState, MenuButton_ItemSelected_RestoresFocusToMenuButton) pass, and the bUnit host-contract test is updated to the marker-based contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b530660c-2232-498c-b083-c1d1d6f7227a
6538952 to
ffd67f9
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
Scroll-button activation misses content-only growth, and the grid auto-fit interaction remains unverified with real pointer input.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- src/Aspire.Dashboard/Resources/ControlsStrings.Designer.cs: Generated file
- src/Aspire.Dashboard/Resources/Layout.Designer.cs: Generated file
Suppressed comments (5)
src/Aspire.Dashboard/Components/Controls/ResourceActions.razor.css:31
--aspire-space-3xsis undefined, so this margin bypasses the design-token system and always uses its fallback. Use the existing--aspire-space-2xstoken defined as 2px.
margin-inline: var(--aspire-space-3xs, 2px);
src/Aspire.Dashboard/wwwroot/js/app.js:622
ResizeObserveronly reports changes to the observed element's box, not itsscrollHeight. These scroll containers keep a fixed viewport while their child grid/log content grows, and the body observer below explicitly ignores content churn, so Structured Logs or Traces can cross the 240px threshold without ever activating the button until an unrelated scroll/resize occurs. Observe the rendered content as well (asinitializeContinuousScrollalready does), and cover growth after registration in the Playwright test.
tests/Aspire.Dashboard.Tests/Integration/Playwright/DashboardInteractionsTests.cs:114- This comment describes proximity gating,
.is-hovered, and a hide delay that do not exist in the current implementation.app.cssmakes.scroll-button.is-visiblevisible without pointer proximity, so the hover is redundant and the test does not assert the behavior described here. Remove the stale within-branch narrative and keep the actual visibility assertion.
src/Aspire.Dashboard/Components/Controls/ResourceActions.razor.css:6 --aspire-space-3xsis not defined by the new token scale (tokens.cssstarts at--aspire-space-2xs), so this always falls back to a hardcoded value and cannot be tuned through the design-token layer. Use the existing 2px token.
This issue also appears on line 31 of the same file.
gap: var(--aspire-space-3xs, 2px);
src/Aspire.Dashboard/Components/Layout/MainLayout.razor:72
- This committed comment references review feedback rather than explaining the current design in isolation. Keep the rationale, but remove the within-branch history so the comment remains meaningful after merge.
No visible text label: the icon plus the dynamic Title/aria-label carry the meaning, and a
written "Collapse sidebar" beside it read as redundant chrome (review feedback). *@
- Files reviewed: 79/89 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. Runs the full test matrix + all jobs (ALL) — a rule matching 'Directory.Packages.props' selects ALL Selection computed for commit |
catalogservice (application) seemingly has less contrast now - it's disabled and needs higher contrast on the text or a different background to show that |
|
I know that's the behavior in the dashboard today, but it's even more strange to me now that the page titles are not clickable. With this redesign, they act more as breadcrumbs (note that I do prefer the updated design), which I would expect to be clickable 
|
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
I don't think this is ready yet. The amendment fixes the menu state, component test, and current axe matrix, but a few blockers remain:
register()can retry forever through already-resolvedcustomElements.whenDefined()promises if a marked trigger is detached before it upgrades. A staged current-head repro blocked the page main thread for more than 45 seconds. Could we bail when!host.isConnectedand bound any retry?- Newly inserted triggers still wait behind the 100 ms document scan before registration. A Fluent harness observed the roleless host retaining invalid
aria-expandedthrough the immediate and 25 ms snapshots. Could we register matching added nodes directly in the mutation callback? - Dark accent buttons measure 4.27:1 on hover and 3.84:1 while active, both below WCAG AA. The code-block contrast test also probes
--neutral-layer-1rather than the production surfaces and misses--hljs-sectionat 4.25:1.
The visible keyboard-focus, Resources toolbar semantics, and off-screen scroll-overlay findings also remain unchanged. Could we fix the retry and WCAG blockers before merging?
|
Documentation was required for the collapsible navigation rail and responsive toolbar changes (signal: See the workflow run for details: https://github.com/microsoft/aspire/actions/runs/30946832615 |
If you're on "Structured logs", and that was link/button...where would you expect to go? You're already on that page, and the logo to the left navs home. The only place that would potentially warrant a bread crumb UX, is traversing the trace details. |
|
🔍 CI Failure Analysis: Transient Infrastructure Failure The CI build failed due to transient infrastructure issues. Failed jobs:
If a rerun was not already requested automatically, visit the workflow run page to rerun the failed jobs manually. |
Resolves conflicts with #18943 (design-token system / UX polish), which replaced AspireMenuButton's `Items` parameter with a lazy `ItemsProvider`. - AspireMenuButton: keep main's ItemsProvider + JS init sequencing, re-add this branch's Disabled and IconStart* parameters. Disabled is now combined with main's "no actionable items" auto-disable via UpdateDisabled(). - Drop this branch's OnOpening parameter: ItemsProvider supersedes it, since both existed to solve lazy menu item loading. - DashboardRunSelect: migrate from Items/OnOpening to ItemsProvider. - Tests: menus now render into MainLayout's FluentMenuProvider rather than inline, so DOM queries move from the run select to the page. Add FluentUISetupHelpers.SetupMenuService for suites that configure FluentUI piecemeal, and register the IMenuService instance instead of resolving it so bUnit's service collection isn't sealed during setup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15a04c2e-0866-4704-9c64-71c66cb1d50b
Description
The Aspire Dashboard's visual styling had grown ad-hoc — colors, spacing, radii, and font sizes were hardcoded and scattered throughout
app.css, which made even small design tweaks slow, risky, and inconsistent across light/dark themes. This PR introduces a design-token layer so the dashboard's look-and-feel can be tuned from one place, refreshes typography, applies a broad UX / information-architecture polish pass, and adds an automated accessibility (axe-core) test suite that guards the result.This is a Dashboard-only change (Blazor components, CSS, JS, localized strings, and tests). There are no public API changes.
Fixes # (issue)
What changed for users
wwwroot/css/tokens.cssdefines the spacing, color, radius, and typography primitives;app.cssnow consumes them. Incremental design changes become fast and consistent across both themes.LICENSE.txtfiles included.Accessibility
A new
tests/Aspire.Dashboard.Tests/Integration/Playwright/AccessibilityTests.csruns axe-core (viaDeque.AxeCore.Playwright) against the dashboard and fails on any serious/critical WCAG 2.x A/AA violation:The suite surfaced, and this PR fixes, real issues it found: message-bar contrast, light-theme code-block contrast, and a missing accessible name on the Settings language selector (
aria-input-field-name, WCAG 4.1.2) — Fluent'sLabelrenders a standalone<label>it never associates with the combobox, so an explicitaria-label(reusing the existing localized string) was added. Full suite: 19/19 passing locally. These are[OuterloopTest]Playwright tests that run off the regular CI loop.Implementation notes
wwwroot/js/app.js/app-theme.jshandle the nav-collapse toggle and persistence (BrowserStorageKeys) plus theme wiring; the collapsed state is stored inlocalStorage(UI preference only — no server or network behavior changes).ControlsStringsandLayoutresources, with.xlfsynced across all 13 locales viaUpdateXlf.Directory.Packages.propsadds theDeque.AxeCore.Playwrightpackage version consumed by the test project.Screenshots / Recordings
Validation
Aspire.Dashboard.Testsbuild clean (0 warnings / 0 errors).AccessibilityTestsfull class: 19/19 pass.Follow-ups (tracked, not blocking)
A few incremental polish items remain and may land as follow-up changes: additional spacing on the top-right header icons, final chrome padding/boxing, and micro-alignment of the page-title left edge.
Checklist
<remarks />and<code />elements on your triple slash comments?