Repository navigation
fix(frontend): give the savings charts a value summary and data table - #1381
Conversation
…table) Chart canvases are role="img", so their aria-label is all a screen reader can announce, and every Home/Purchases chart uses a fixed label with no values. lib/chart-a11y.ts adds setChartSummary (a data-driven aria-label) and renderChartTable / removeChartTable, a collapsed "View as table" <details> listing every plotted value with column and row headers. It is inserted after an anchor the caller picks so it never sits inside a box Chart.js measures, keeps its open state across data refreshes, and builds cells with textContent only. Refs LeanerCloud#744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Home "Savings over time" and "Potential savings range per service" charts and the Purchases savings-history chart now set an aria-label that states what is plotted (count, total, largest service or latest cumulative value) and render a "View as table" disclosure with every value; the table is removed when the chart switches to its empty or error state. The per-service table lists every service, including any beyond the 10 bars drawn. The per-service chart's stacked layers also get a 2px card-colored gap on top of each segment, so adjacent layers are separated by more than their shade difference. Refs LeanerCloud#744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 8 billable files and costs up to $2.00. Or wait 59 minutes for your next included review. View limit details
Comment |
Conflicts: - frontend/src/dashboard.ts (imports): main's formatTrendTooltipTitle import (LeanerCloud#1323) plus LeanerCloud#744's chart-a11y import. - frontend/src/dashboard.ts (chart constants): kept LeanerCloud#744's SEGMENT_GAP and table ids and main's SAVINGS_LAYER_COLORS (LeanerCloud#1335). Took main's removal of the per-service palette helpers (serviceBarColor, parseHexColor, darkenHexColor, legend swatch constants). SEGMENT_GAP's comment now says the gap separates layers by an edge as well as by layer colour, since main made the layers differ by hue. - frontend/src/dashboard.ts (renderSavingsByService): kept LeanerCloud#744's summary label and "View as table" table; took main's removal of the per-service shade derivation. Each dataset keeps main's layer colour plus LeanerCloud#744's ...SEGMENT_GAP (auto-merged). - frontend/src/styles/charts.css: both appended blocks kept, LeanerCloud#744's .chart-table rules then main's Trends tab rules (LeanerCloud#1317). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- frontend/src/dashboard.ts (loadSavingsTrendChart empty and error
paths): kept this branch's removeChartTable(SAVINGS_TREND_TABLE_ID) and
took main's removal of attachSparkline('ytd', []) there; since LeanerCloud#1344 the
YTD sparkline has its own series loaded by loadYtdSparkline(), so the
trend chart's empty/error state no longer clears it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - frontend/src/dashboard.ts (imports): took main's widened chart-utils import (formatSparseTrendMessage, MIN_TREND_POINTS from LeanerCloud#1334) and kept this branch's lib/chart-a11y import. - frontend/src/dashboard.ts (loadSavingsTrendChart): kept this branch's removeChartTable in the no-data branch and main's new sparse-data branch (LeanerCloud#1334). The sparse branch also hides the canvas, so it now calls removeChartTable too, so a table from an earlier render does not outlive its chart (the empty and error states already do this). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e state Main's LeanerCloud#1334 shows a "not enough history yet" message instead of a chart when the savings trend has fewer than MIN_TREND_POINTS (2) points. The LeanerCloud#744 test that checks the trend table is removed on empty data started from a single point, which is now the sparse state with no chart and no table. It now starts from two points, and a new case checks the table is also removed when the data drops to the sparse state. Refs LeanerCloud#744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE Blocking:
|
|
Local verification at
|
|
Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at |
Brings in LeanerCloud#1365, LeanerCloud#1381, LeanerCloud#1378 and LeanerCloud#1389. No conflicts; index.html auto-merged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Home and Purchases savings charts are
<canvas role="img">elements with fixedaria-labels, so a screen-reader user heard a generic description and could not get a single value out of them, and the stacked layers of the per-service chart differed only by shade. This PR gives each chart a data-driven summary label and a "View as table" disclosure with every value, and separates the stacked layers with a 2px gap.Refs #744
Partial: the layers still share one hue per service. Distinct per-layer colors are in the branch for #189, which edits the same datasets; doing both here would conflict.
What changed
frontend/src/lib/chart-a11y.ts:setChartSummary(data-drivenaria-label) andrenderChartTable/removeChartTable, a collapsed "View as table"<details>with a caption, column headers and row headers. Cells are built withtextContentonly, the disclosure is inserted after an anchor outside Chart.js's sizing box, and it keeps its open state on refresh.frontend/src/dashboard.ts: the "Potential savings range per service" label states the service count, total and largest service, and its table lists Current / Committed, Lowest option, Upside and Total for every service (including any beyond the 10 bars drawn). The "Savings over time" label states the range, latest cumulative value and point count, with a Date / Cumulative savings table. Both tables are removed in empty and error states. The per-service datasets get a 2px top border in the card color, so adjacent layers are separated by a gap.frontend/src/modules/savings-history.ts: the same summary and table (Date / Period savings / Cumulative savings) for the Purchases chart, placed after its.chart-container.frontend/src/styles/charts.css: disclosure and table styles (scrolls inside a 320px box).chart-a11y.test.ts(6), new cases indashboard.test.ts(7) andsavings-history.test.ts(2), and a newtests-e2e/chart-text-alternative.spec.ts.Commits:
edf1c487(helper),44678ef8(charts).Why this approach
A native
<details>disclosure works with the keyboard and screen readers without custom JS, stays out of the way when collapsed, and also helps sighted users who can't separate the colors (the issue's "View as table" option). The ladder timeline already uses the same pattern (dynamicaria-labelplus a table), so this follows an existing convention in the repo. Putting the table after the canvas's own box avoids Chart.js resize loops.Screenshots
Before is main (
a14dc8f2), after is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API. The yellow dashed box is injected by the screenshot script to print the canvas'saria-label(what a screen reader announces); it is not part of the app.Verification
Run on this branch's head
44678ef8on top of maina14dc8f2, fromfrontend/:npm run lint: 0 errors (existing warnings only). Typecheck and build pass.--randomize.e2e-local.sh): 159 passed, including the newchart-text-alternative.spec.ts(the per-service label names the service count; the table opens with the keyboard and lists every service by row header; the trend chart has a label and table).scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Notes for review
aria-labelinindex.htmlis still the initial value before data loads; it is replaced on every render.Overlaps
renderSavingsByServicedatasets (colors) and the sameindex.htmlaria-labelline: textual conflicts, best merged before this one.dashboard.tsandsavings-history.ts(new Chart->upsertChart) and adds a generation guard toloadSavingsTrendChart: conflicts near the code added here, easy to resolve.loadDashboardonly.Note for the UI owner
This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.
🤖 Generated with Claude Code