fix(charts): dark-mode readability audit — pie labels, radar, choropleth, map controls, graph edges (#1154) - #1177
Merged
Conversation
…eth, map controls, graph edges (#1154) Storybook screenshot sweep of all 92 chart stories in both themes surfaced the remaining dark-mode readability failures after the bar-label fix (#1167). Fixed, each verified before/after: - Pie slice labels: ECharts' default dark-fill + light-halo label style (pixel-confirmed #333 with white stroke) was black-on-dark. Theme-level `pie.label` → foreground, no halo — same fix as bar. - Radar indicator labels + visualMap legend text: neither inherits the global textStyle; their dim defaults were hard to read on the dark canvas. Explicit muted-foreground per theme. - Choropleth region labels: invisible on dark no-data regions. Dark mode now uses the treemap white-with-shadow fill-label pattern; light mode keeps the default dark text (an unconditional white label washed out on the pale light map — caught in verification and made theme-conditional). - Leaflet attribution: the `.dark` background override tied Leaflet's own `.leaflet-container .leaflet-control-attribution` on specificity and lost on load order (leaflet.css is imported lazily by map-chart), so the strip stayed white with amber links. Selector now includes `.leaflet-container` to win outright. Also styled the disabled zoom button, whose light-grey Leaflet default glared on the dark stack. - Graph edges: NVL's default relationship grey nearly vanishes on the dark canvas — uncolored edges get an explicit mid-grey in dark mode, and explicit edge colors are normalized hsl→hex for NVL (#1157 class). Known minors deliberately left (noted on #1154): sankey ribbon tint, gauge low-value track, line-chart axis-name/legend overlap (both themes, layout), map marker translucency over dark tiles, circle-packing label occlusion. Tests: theme-defaults (pie/radar/visualMap, both themes), graph-chart edge color mapping (dark default + hsl→hex). Component suite 1894 green; theme + charts E2E pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
alfredo1996
added
area:a11y
WalkthroughDark-mode text and styling readability improvements across chart theming (pie labels, radar axisName, visualMap textStyle), choropleth map labels, graph edge colors, and Leaflet CSS selectors, with accompanying unit tests. ChangesDark mode readability fixes
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@component/src/charts/__tests__/theme-defaults.test.ts`:
- Around line 92-95: The assertions in theme-defaults.test.ts are passing a
duplicate second argument to Jest’s toBe matcher, which causes a TypeScript
compile error. Update the affected expectations in the axisName and visualMap
tests so each expect(...).toBe(...) call receives only the single intended
value, using the existing theme.legend textStyle color expression once in each
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7069edfe-4ce8-4156-a390-3918ba327b82
📒 Files selected for processing (6)
component/src/charts/__tests__/graph-chart.test.tsxcomponent/src/charts/__tests__/theme-defaults.test.tscomponent/src/charts/choropleth-chart.tsxcomponent/src/charts/graph-chart.tsxcomponent/src/charts/theme.tscomponent/src/index.css
Comment on lines
+92
to
+95
| expect(axisName.color).toBe( | ||
| (theme.legend as { textStyle: { color: string } }).textStyle.color, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate argument passed to toBe() — TS compile error.
Both new assertions pass the same expression twice into .toBe(...), e.g.:
expect(axisName.color).toBe(
(theme.legend as { textStyle: { color: string } }).textStyle.color,
(theme.legend as { textStyle: { color: string } }).textStyle.color,
);
toBe takes a single argument; TypeScript will reject the extra argument, breaking the build/test run. Same duplication in the visualMap test.
🐛 Proposed fix
expect(axisName.color).toBe(
- (theme.legend as { textStyle: { color: string } }).textStyle.color,
(theme.legend as { textStyle: { color: string } }).textStyle.color,
); expect((vm.textStyle as Record<string, unknown>).color).toBe(
- (theme.legend as { textStyle: { color: string } }).textStyle.color,
(theme.legend as { textStyle: { color: string } }).textStyle.color,
);Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@component/src/charts/__tests__/theme-defaults.test.ts` around lines 92 - 95,
The assertions in theme-defaults.test.ts are passing a duplicate second argument
to Jest’s toBe matcher, which causes a TypeScript compile error. Update the
affected expectations in the axisName and visualMap tests so each
expect(...).toBe(...) call receives only the single intended value, using the
existing theme.legend textStyle color expression once in each assertion.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1154 (the bar-label part landed in #1167; this completes the audit).
Method
Screenshot sweep of all 92 chart stories × 2 themes via Storybook + Playwright, reviewed by three parallel readers, each finding root-caused in code and re-verified with after screenshots.
Fixes (each verified before/after)
#333+ white halo)pie.label→ foreground, no halo (same as the bar fix)radar.axisNamedoesn't inherittextStylevisualMap.textStyledoesn't inherit either.darkoverride tied Leaflet's.leaflet-container .leaflet-control-attributionon specificity and lost on load order (leaflet.css imported lazily).leaflet-container; also styled the glaring disabled zoom buttonDeliberately left as minors (noted on the issue)
Sankey ribbon tint, gauge low-value track, line-chart axis-name/legend overlap (both themes — layout, not contrast), map marker translucency over dark tiles, circle-packing label occlusion.
Tests
theme+chartsE2E pass (2 unrelated login-timeout flakes, pass in CI).🤖 Generated with Claude Code
Summary by CodeRabbit