refactor(charts): describe bar series as objects, not parallel maps - #8470
refactor(charts): describe bar series as objects, not parallel maps#8470talissoncosta wants to merge 2 commits into
Conversation
Series drawn with an SVG fill-opacity had no matching swatch: colours are CSS var() strings, so a className can't carry a per-series alpha. Adds an optional opacity, and a story alongside the other prop variants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BarChart took four maps keyed by dataKey (colours, labels, stack ids, fill opacities) with nothing tying the keys together, so a missing entry silently dropped a bar's colour or stack. One BarSeries per series replaces all four, and the legend now reads the same array the bars do, so a faded swatch can't drift from its faded bar. Two props go with it. `grouped` was unreachable: side by side is distinct stack ids, which is what the conversion chart's daily mode already did. The tooltip's formatter and its total flag become one object, with a formatted value hiding the non-additive total by default rather than asking every caller to remember. Existing charts keep their shared default stack, so nothing moves visually. toBarSeries bridges callers still holding maps from buildChartColorMap or useEnvChartProps, which LineChart continues to read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Bar charts now use structured series metadata for labels, colours, stacking, opacity, and tooltips. The functional migration appears ready, with a low-risk cleanup needed to align new chart-module imports with frontend conventions. 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 |
github-actions
Bot
added
front-end
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 8ca7d968-5853-430a-bfcc-d0108efd587b
📒 Files selected for processing (14)
frontend/documentation/components/BarChart.stories.tsxfrontend/documentation/components/ColorSwatch.stories.tsxfrontend/web/components/ColorSwatch.tsxfrontend/web/components/charts/BarChart.tsxfrontend/web/components/charts/index.tsfrontend/web/components/charts/toBarSeries.tsfrontend/web/components/charts/types.tsfrontend/web/components/experiments/results/ExperimentConversionRateCard/ExperimentConversionRateCard.tsxfrontend/web/components/experiments/results/__tests__/deriveConversionRate.test.tsfrontend/web/components/experiments/results/deriveConversionRate.tsfrontend/web/components/feature-page/FeatureNavTab/FeatureAnalytics.tsxfrontend/web/components/organisation-settings/usage/OrganisationUsage.container.tsxfrontend/web/components/organisation-settings/usage/components/SingleSDKLabelsChart.tsxfrontend/web/components/pages/usage/components/UsageOverTime/UsageOverTime.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| import ChartTooltip from './ChartTooltip' | ||
| import { ChartDataPoint } from './types' | ||
| import { BarSeries, ChartDataPoint } from './types' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the relative chart-module imports.
Lines 14-15 use relative paths. Use components/charts/ChartTooltip and components/charts/types instead.
As per coding guidelines, “Use only common/, components/, and project/ import paths; do not use relative imports.”
Proposed fix
-import ChartTooltip from './ChartTooltip'
-import { BarSeries, ChartDataPoint } from './types'
+import ChartTooltip from 'components/charts/ChartTooltip'
+import { BarSeries, ChartDataPoint } from 'components/charts/types'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ChartTooltip from './ChartTooltip' | |
| import { ChartDataPoint } from './types' | |
| import { BarSeries, ChartDataPoint } from './types' | |
| import ChartTooltip from 'components/charts/ChartTooltip' | |
| import { BarSeries, ChartDataPoint } from 'components/charts/types' |
Source: Coding guidelines
| @@ -0,0 +1,17 @@ | |||
| import { BarSeries } from './types' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use frontend aliases for the new chart module paths.
The changed chart modules introduce relative paths, which violates the frontend import-path rule.
frontend/web/components/charts/toBarSeries.ts#L1-L1: usecomponents/charts/typesinstead of./types.frontend/web/components/charts/index.ts#L7-L8: usecomponents/charts/toBarSeriesandcomponents/charts/typesinstead of relative paths.
As per coding guidelines, frontend/**/*.{js,jsx,ts,tsx} must use only common/, components/, and project/ import paths. Do not use relative imports.
📍 Affects 2 files
frontend/web/components/charts/toBarSeries.ts#L1-L1(this comment)frontend/web/components/charts/index.ts#L7-L8
Source: Coding guidelines
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #8463, targeting that branch so the diff is only my part. A suggestion, not a blocker.
BarChartgained seven props in #8463, five with a single caller and two with none. This changes the shape rather than trimming the list:BarSeries(key,label,colour,stackId?,opacity?) replacescolorMap,seriesLabels,stackMapandopacityMap, so the legend reads the same array the bars do.groupedgoes, since side by side is just distinct stack ids.yAxisgoes until something passes it.formatValuehides the non-additive total by default.15 props down to 8, and the conversion card's call site from six to two.
ColorSwatchgains an optionalopacityso the faded legend swatch is the shared component, andtoBarSeriesbridges callers still holding maps, whichLineChartcontinues to read. Existing charts keep their default shared stack, so nothing moves visually.Left out on purpose:
REST_SUFFIXand the card's key parsing, which depend on whetherbuildConversionRateChartDatastays.How did you test this code?
npm run test:unit: 43 suites, 614 tests pass.npm run typecheck: 938 errors, same as the base branch. Zero delta.PartOfWholeStacks(BarChart) andFaded(ColorSwatch) stories, so the stacks, the fade and the legend can be checked in Storybook.