[dark mode] line chart examples#6916
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a per-test ColorMode provider and test fixture, updates visual tests to use the fixture and disable chart animations, converts many LineChart examples to CSS-variable theming (some now accept isAnimationActive), expands theme tokens, and updates StackBlitz index payload. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as rgba(66,133,244,0.5) Test Runner
participant Fixtures as rgba(34,139,34,0.5) fixtures.mount
participant Provider as rgba(128,0,128,0.5) TestColorModeProvider
participant Store as rgba(255,165,0,0.5) defineColorModeStore
participant Component as rgba(220,20,60,0.5) MountedComponent
TestRunner->>Fixtures: request mount(MountedComponent)
Fixtures->>Provider: render wrapper around component
Provider->>Store: create per-test ColorMode store
Provider->>Component: provide ColorModeStore via ColorModeProvider
Component-->>TestRunner: render and produce screenshot
Note right of Provider: on unmount\ndispose store
Provider->>Store: dispose store
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6916 +/- ##
==========================================
+ Coverage 90.12% 90.18% +0.06%
==========================================
Files 526 526
Lines 39183 39557 +374
Branches 5423 5423
==========================================
+ Hits 35312 35676 +364
- Misses 3862 3872 +10
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportBundle size has no change ✅ |
794f3cc to
527b511
Compare
|
I need to study how to support storybook also |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
www/src/docs/exampleComponents/LineChart/CustomizedLabelLineChart.tsx (1)
59-67: ReplaceanywithBaseTickContentPropsfrom recharts.
Line 59 usesany, which violates the TS guideline. Recharts exportsBaseTickContentPropsfrom the public API which includesx,y, andpayloadproperties. Use this type instead ofanyto keep it properly typed and aligned with the public API.♻️ Suggested fix
-import { LineChart, Line, XAxis, YAxis, CartesianGrid, Tooltip, Legend, ResponsiveContainer } from 'recharts'; +import { LineChart, Line, XAxis, YAxis, CartesianGrid, Tooltip, Legend, ResponsiveContainer, BaseTickContentProps } from 'recharts'; -const CustomizedAxisTick = ({ x, y, payload }: any) => { +const CustomizedAxisTick = ({ x, y, payload }: BaseTickContentProps) => { return ( <g transform={`translate(${x},${y})`}> <text x={0} y={0} dy={16} textAnchor="end" fill="#666" transform="rotate(-35)"> - {payload.value} + {payload?.value} </text> </g> ); };
🧹 Nitpick comments (2)
www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsx (1)
37-43: Avoid usinganytype in TypeScript.The
TooltipContentProps<any, any>usesanyfor both generic parameters, which violates the coding guidelines. Consider using more specific types orunknownif the actual types are not known.As per coding guidelines, prefer
unknownoveranyand refine the type.♻️ Suggested improvement
-const CustomTooltip: FC<TooltipContentProps<any, any>> = ({ active, payload }) => { +const CustomTooltip: FC<TooltipContentProps<number, string>> = ({ active, payload }) => {Alternatively, if the exact types are uncertain:
-const CustomTooltip: FC<TooltipContentProps<any, any>> = ({ active, payload }) => { +const CustomTooltip: FC<TooltipContentProps<unknown, unknown>> = ({ active, payload }) => {www/src/docs/exampleComponents/LineChart/LineChartHasMultiSeries.tsx (1)
44-57: Consider using distinct colors for each series.All three series use the same
var(--color-chart-1)stroke color, which may make them visually indistinguishable. Consider cycling through chart color tokens (e.g.,--color-chart-1,--color-chart-2,--color-chart-3) based on the series index to improve visual differentiation.💡 Suggested improvement
{series.map((s, index) => ( <Line dataKey="value" data={s.data} name={s.name} key={s.name} - stroke="var(--color-chart-1)" + stroke={`var(--color-chart-${index + 1})`} dot={{ fill: 'var(--color-surface-base)', }}
|
Storybook already has dark mode included |
|
But its not in sync with the color-mode algo, I will see in a future PR btw |
|
what do you think about the fixture? I will need to use it wrap all www related VR tests |
|
|
a817561 to
4e7b138
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@www/src/docs/exampleComponents/LineChart/LineChartHasMultiSeries.tsx`:
- Around line 44-57: The Line components rendered in series.map currently all
use the same stroke "var(--color-chart-1)" so multi-series lines are
indistinguishable; update the map callback to include the series index (e.g.,
series.map((s, i) => ...)) and set each Line's stroke to a token derived from
the index (for example "var(--color-chart-{i+1})" or a small array of tokens) so
each series rendered by the Line component has a distinct color; keep the
existing props (dataKey, data, name, key, dot, activeDot) but replace the
hard-coded stroke with an index-driven token.
PavelVanecek
left a comment
There was a problem hiding this comment.
Let's find some solution for the VR test that doesn't include maxDiffPixelRatio.
What was the failure? Do you run the tests in Docker?
|
Dark mode sounds simple until it doesn't, right @cloud-walker ? |
|
we will found a way sooner or later 😄 |
|
Why do we need to support the stackblitz examples? Just let them go back to light mode. Our website is being themed, not recharts (or at least not in this PR) and stackblitz is outside of that |
At least we need to pass the CSS variables, the code is the same of the docs, but yeah we can keep the light mode by default. But why you think its not a good idea to support color-mode propagation? Its a plus to me that the end user can benefit from it also there. That said, merging it now would worsen the experience for not yet migrated chart, so yeah, better keep light mode at least for now.
Yeah, lets continue the recharts itself theming discussione elsewhere, I agree, but @ckifer what to do you think about the strategy to continue the docs theming? So to recap, I need to:
|
|
Not saying that we shouldn't support theme in stackblitz or we need to keep it in light mode, only that if we want to do that it doesn't have to be in this PR for the sake of getting this merged |
|
Okay so the example list fails on timeout, looks like some animation never settles? Or github runner is too slow? The vertical line chart appears to be 1 pixel off. Let me try to checkout your branch and see if I can reproduce the failure. 
|
|
I can see the same failure on my laptop so let's see if this helps |
- Updated CartesianGrid, XAxis, YAxis, and Tooltip components across various LineChart examples to use color variables for better theming. - Modified Line components to utilize color variables for strokes and active dots, enhancing visual consistency. - Improved accessibility and readability by adjusting stroke colors and adding active dot styles. - Refactored common chart elements into reusable components to reduce redundancy and improve maintainability. - Updated snapshot images for LineChart examples to reflect the new styles and ensure visual accuracy in tests.
…mlCode to support dynamic color modes
…n and BiaxialLineChart with maxDiffPixelRatio
…t mode by default
…eChartWithSpecifiedDomain components
c697150 to
b1f40e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
www/src/components/Shared/StackBlitzLink.tsx (1)
159-161: Hardcodedmode: 'light'aligns with the PR strategy to defer dark-mode StackBlitz support.Consider adding a brief comment at line 160 noting the light-mode lock is intentional and temporary, so future contributors know to wire it to the actual color mode when ready.
💡 Suggestion
- 'index.html': indexHtmlCode({ title, mode: 'light' }), + // TODO: wire `mode` to the active color mode once dark-mode StackBlitz support is ready + 'index.html': indexHtmlCode({ title, mode: 'light' }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/src/components/Shared/StackBlitzLink.tsx` around lines 159 - 161, Add a short inline comment next to the files entry where indexHtmlCode is called (the 'index.html': indexHtmlCode({ title, mode: 'light' }) line in StackBlitzLink.tsx) stating that mode: 'light' is intentionally hardcoded as a temporary decision to defer dark-mode support and should be wired to the app color mode (e.g., from context or a prop) when dark-mode StackBlitz is implemented; place the comment immediately above or beside the call so future contributors understand this is deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-vr/tests/www/dark-mode.spec-vr.tsx`:
- Around line 13-15: The current test.beforeEach only calls page.emulateMedia({
reducedMotion: 'reduce' }) which does not stop Recharts animations; update the
setup to explicitly disable Recharts by adding an initialization script before
pages load that sets a global flag or stubs Recharts animation behavior (for
example, use page.addInitScript to set window.__RECHARTS_DISABLE_ANIMATION =
true or override requestAnimationFrame/animation functions) so Recharts will
render without animations; keep the existing test.beforeEach and add this
explicit disabling step (reference test.beforeEach, page.emulateMedia,
page.addInitScript and the global flag name) so screenshot tests are
deterministic.
In `@www/src/docs/exampleComponents/LineChart/CompareTwoLines.tsx`:
- Around line 910-911: The Portfolio line's activeDot.fill still uses the
hardcoded color constant (color) while its stroke uses the CSS variable
var(--color-chart-1), causing a mismatch in dark mode; update the Portfolio
Line's activeDot.fill to match its stroke theming (use the same CSS variable or
the same color variable used for the stroke) so the hover dot and line stay
visually consistent (mirror the pattern used by the Compare line where
compareColor is used for both stroke/gradient and activeDot.fill).
---
Duplicate comments:
In `@test-vr/tests/www/dark-mode.spec-vr.tsx`:
- Around line 37-48: The VR snapshot is flaky because SimpleLineChart renders
with animations enabled; either update the SimpleLineChart component to accept
an isAnimationActive prop and default it to false (modify the SimpleLineChart
React component to read isAnimationActive and pass it into the underlying chart
rendering), or disable animations in the test by mounting <SimpleLineChart
isAnimationActive={false} /> in the dark-mode.spec-vr.tsx test; make sure the
prop name is exactly isAnimationActive so it matches other examples like
LineChartExample/BarChartExample/PieChartExample.
---
Nitpick comments:
In `@www/src/components/Shared/StackBlitzLink.tsx`:
- Around line 159-161: Add a short inline comment next to the files entry where
indexHtmlCode is called (the 'index.html': indexHtmlCode({ title, mode: 'light'
}) line in StackBlitzLink.tsx) stating that mode: 'light' is intentionally
hardcoded as a temporary decision to defer dark-mode support and should be wired
to the app color mode (e.g., from context or a prop) when dark-mode StackBlitz
is implemented; place the comment immediately above or beside the call so future
contributors understand this is deliberate.
PavelVanecek
left a comment
There was a problem hiding this comment.
I am still secretly hoping for the Theme provider one day
|
merging this before conflicts |
Description
based on #6882 discoveries, we have to:
So I'm preparing a smaller PR to solve those first
Related Issue
#6825
Motivation and Context
Otherwise stackblitz does not work anymore
How Has This Been Tested?
Playwright and manually
Screenshots (if appropriate):
Are in the diff
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests