Repository navigation
fix(frontend): color the per-service savings chart by layer - #1335
Conversation
…gend matches The legend names three stacked layers (Current / Committed, Lowest option, Upside) but the bars were colored per service, with the layers only as darker/solid/translucent shades of each service's hue. The neutral gray legend swatches matched no bar, and within a bar the layers differed only by shade. The x axis already names every service, so hue now encodes the layer: each layer is one color on every bar, and Chart.js's default legend draws exactly that color. The three colors are slots 1-3 of a validated categorical palette (distinct under common color-vision deficiencies) and are keyed on the layer, so they never change with a service's rank. The redundant "Service" x-axis title is dropped. The per-service palette helpers and the custom legend generator are removed with their tests. Refs LeanerCloud#189 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 3 billable files and costs up to $0.75. Or wait 59 minutes for your next included review. View limit details
Comment |
|
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 |
Conflicts: - frontend/src/dashboard.ts: main (LeanerCloud#1335, Refs LeanerCloud#189) recoloured the per-service savings chart by layer (one hex per stacked layer in SAVINGS_LAYER_COLORS, Chart.js default legend) and removed the per-service palette, darkenHexColor and the custom legend; this branch had moved those per-service colours onto --cudly-chart-* tokens. Kept main's layer design and this branch's tokens: the three layer colours are now --cudly-chart-layer-current/-lowest/-upside tokens (main's exact hex values) read at render time by a new savingsLayerColors(), which replaces the SAVINGS_LAYER_COLORS constant. Dropped serviceBarColor, KNOWN_SERVICE_COLOR_INDEX, legendSwatchColors, darkenHexColor and the parseHexColor re-export, as main did. Import block: main's formatTrendTooltipTitle plus the chart-theme helpers still used (applyChartDefaults, chartColor, withAlpha). - frontend/src/styles/base.css: :root token block. Kept both main's motion tokens (LeanerCloud#725) and this branch's chart tokens; replaced the now-unused --cudly-chart-legend-dark/-mid/-border tokens with the three layer tokens. - frontend/src/lib/chart-theme.ts (follows from the above): fallbacks swap the legend tokens for the layer tokens; comments updated. - frontend/src/__tests__/chart-theme.test.ts, dashboard.test.ts (follow from the above): the themed-palette test now overrides a layer token via savingsLayerColors(); main's layer-colour tests call savingsLayerColors() instead of SAVINGS_LAYER_COLORS, assertions unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 (import): main (LeanerCloud#1323) added formatTrendTooltipTitle to the chart-utils import, this branch added upsertChart. Import both. - frontend/src/dashboard.ts (renderSavingsByService): main (LeanerCloud#1335) dropped the per-service bgColors/darken/upside shade block in favour of the fixed SAVINGS_LAYER_COLORS; this branch replaced new Chart(...) with upsertChart(...). Took main's layer colours and kept upsertChart, so the per-service chart still updates in place. - frontend/src/modules/chart-utils.ts: both sides appended a helper after formatTrendAxisTick (main's formatTrendTooltipTitle, this branch's upsertChart). Kept both, main's first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Home "Potential savings range per service" chart's legend names three stacked layers (Current / Committed, Lowest option, Upside), but each bar was drawn in its own service color, with the layers as darker, solid and translucent shades of that color. The neutral gray legend swatches matched none of the bars. This PR colors the chart by layer, so each layer is one color on every bar and the legend shows exactly that color, and drops the redundant "Service" axis title.
Closes #189
What changed
frontend/src/dashboard.ts: replaced the per-service palette (SERVICE_BAR_COLORS,KNOWN_SERVICE_COLOR_INDEX,serviceBarColor,parseHexColor,darkenHexColor,LEGEND_SWATCH_*) withSAVINGS_LAYER_COLORS, one color per layer: Current / Committed#2a78d6(blue), Lowest option#eb6834(orange), Upside#1baf7a(aqua). Each dataset gets a singlebackgroundColor, so Chart.js's default legend draws the bar color, and the customgenerateLabelsis removed. The x-axis "Service" title is removed.frontend/src/index.html: dropped "darker" from the static canvasaria-label.frontend/src/__tests__/dashboard.test.ts: removed the per-service color tests and added 4 for the new encoding (one color per layer equal to the legend, three distinct colors, colors unchanged when a service changes rank, no x-axis title).Why this approach
The issue asks to pick one encoding. The x axis already names every service, so coloring by service repeats it, while the layers (what the legend explains) had no clear encoding. Coloring by layer makes the legend match the bars, removes the "shade only" problem that #744 also reports, and keeps the goal of #700 (colors never follow rank, because they follow the layer). The colors are the first three slots of a palette checked for color-vision-deficiency separation (CVD delta-E >= 9.2 and normal-vision delta-E >= 24 between every pair).
The alternative, keeping per-service hues and distinguishing layers with hatching or patterns plus a pattern legend, would keep the #700 design but needs canvas patterns and still spends hue on something the axis already shows.
Screenshots
Before is main (
a14dc8f2), after is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API, with 4 services, 3 of them with existing commitments, so every bar shows all three layers.Before: bars are colored per service (blue, green, yellow, red) in three shades, the gray legend swatches match no bar, and there is a redundant "Service" axis title. After: each layer is one color on every bar (blue current, orange lowest option, aqua upside), the legend shows the same colors, and the axis title is gone.
Verification
Run on this branch's head
38c6b194on top of maina14dc8f2:npm run lint: 0 errors (existing warnings). Typecheck and build pass.--randomize.e2e-local.sh): 157 passed. No new Playwright spec: this is a Chart.js config change, the jest tests assert the config, and the screenshots show the rendered result.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Notes for review
Overlaps
renderSavingsByServiceblock (a 2px gap border per dataset, plus a table andaria-label) and the sameindex.htmlaria-labelline. Expect textual conflicts; merge this one first, then fix(frontend): dashboard charts have no text alternative and layers differ only by shade #744.new CharttoupsertChart): small conflict.dashboard.ts.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