fix: sync pie tooltip/legend color with per-sector fill#6977
fix: sync pie tooltip/legend color with per-sector fill#6977PavelVanecek merged 14 commits intorecharts:mainfrom
Conversation
|
|
||
| if (Array.isArray(tooltipPayload)) { | ||
| tooltipPayload.forEach(item => { | ||
| const preserveItemColor = settings?.chartType === 'pie'; |
There was a problem hiding this comment.
Why only Pie? What happens if we allow this for all charts?
There was a problem hiding this comment.
I originally scoped this to Pie to minimize behavior changes, since the issue report was Pie-specific.
That said, preserving color/fill for all chart types may be better for consistency.
I tried the global version (preserving item.color/item.fill for all charts) and reran related tooltip/selector tests, and they all passed.
If you prefer, I can update this PR either way:
- Keep Pie-only scope (minimal change)
- Apply globally across chart types (consistency-first)
There was a problem hiding this comment.
Let's try all charts. Unit tests will pass either way because we don't test colour in unit tests. VR tests will show the difference.
There was a problem hiding this comment.
Updated based on your feedback:
I removed the Pie-only check.
Now combineTooltipPayload keeps each item's color/fill for array payloads in all chart types.
I also updated the related tooltip test expectations.
There was a problem hiding this comment.
@PavelVanecek
Follow-up update:
I kept item-level color/fill for array payloads, and added fallback behavior when item values are missing:
color: item.color ?? settings?.colorfill: item.fill ?? settings?.fill
This keeps the all-charts behavior while preserving stable existing defaults. (commit: de86534)
I reran VR tests, and diffs appear across multiple chart examples (not only Pie), which is consistent with applying this tooltip color/fill behavior across chart types.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6977 +/- ##
==========================================
- Coverage 90.45% 90.09% -0.37%
==========================================
Files 517 523 +6
Lines 38618 39002 +384
Branches 5347 5388 +41
==========================================
+ Hits 34933 35137 +204
- Misses 3676 3856 +180
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 2.87kB (0.1%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
|
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:
WalkthroughPie now threads per-sector and activeShape fill/colors into tooltip payloads by deriving sectorColor during sector computation and passing activeShape into tooltip entry settings so tooltip items include color and fill fields. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@storybook/stories/Examples/Pie/PieColorSync.stories.tsx`:
- Around line 3-5: The story imports Recharts internals from '../../../../src'
and '../../../../src/polar/Pie' instead of the public package; update imports
for components (Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip) to
come from the public 'recharts' entrypoint and replace the internal type
PieSectorDataItem with a publicly exported type or derive it from public props
(or add a re-export in the library if absent); also import RechartsHookInspector
from the storybook addon package rather than internal paths so all references
(Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip, PieSectorDataItem,
RechartsHookInspector) use the public API.
In `@test/polar/Pie/Pie.spec.tsx`:
- Around line 218-220: The test currently uses a TypeScript assertion
"(tooltipItem as HTMLElement)" which violates the no-`as` rule; instead narrow
the runtime type after selecting the element: keep the querySelector and
assertNotNull(tooltipItem) (or replace it with a runtime check), then add a
guard like "if (!(tooltipItem instanceof HTMLElement)) throw new Error(...)" or
use an existing helper (e.g. assertIsHTMLElement) so you can safely call
tooltipItem.style.color in the expect; update the assertion line to use the
narrowed tooltipItem without any `as` cast.
🧹 Nitpick comments (1)
test/state/selectors/selectors.spec.tsx (1)
1418-1421: Consider asserting specific color values for stronger regression coverage.Using
expect.any(String)avoids coupling to the default palette, which is reasonable. However, it won't catch regressions where sectors receive wrong or swapped colors. If the default palette is stable, matching against actual expected hex values (e.g., fromRECHARTS_DEFAULT_COLORSor equivalent) would strengthen the test.
| import { Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip } from '../../../../src'; | ||
| import { PieSectorDataItem } from '../../../../src/polar/Pie'; | ||
| import { RechartsHookInspector } from '../../../storybook-addon-recharts'; |
There was a problem hiding this comment.
Use public API imports for Recharts components/types.
Storybook files should import from the public entry point (recharts), not src or internal module paths. If PieSectorDataItem isn’t exported, consider re-exporting it or deriving the type from public props.
🔧 Suggested direction
- import { Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip } from '../../../../src';
- import { PieSectorDataItem } from '../../../../src/polar/Pie';
+ import { Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip, PieSectorDataItem } from 'recharts';As per coding guidelines: "All imports from recharts must use the public API entry point (recharts), not internal paths like recharts/types/* or recharts/src/*".
🤖 Prompt for AI Agents
In `@storybook/stories/Examples/Pie/PieColorSync.stories.tsx` around lines 3 - 5,
The story imports Recharts internals from '../../../../src' and
'../../../../src/polar/Pie' instead of the public package; update imports for
components (Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip) to come
from the public 'recharts' entrypoint and replace the internal type
PieSectorDataItem with a publicly exported type or derive it from public props
(or add a re-export in the library if absent); also import RechartsHookInspector
from the storybook addon package rather than internal paths so all references
(Legend, Pie, PieChart, ResponsiveContainer, Sector, Tooltip, PieSectorDataItem,
RechartsHookInspector) use the public API.
test/polar/Pie/Pie.spec.tsx
Outdated
| if (!(tooltipItem instanceof HTMLElement)) { | ||
| throw new Error(`Expected instance of HTMLElement, instead received: [${tooltipItem}]`); | ||
| } | ||
| expect(tooltipItem.style.color).toBe('rgb(255, 0, 0)'); |
There was a problem hiding this comment.
expect().toHaveStyle() also works
| stroke: '#fff', | ||
| tooltipPayload: [ | ||
| { | ||
| color: expect.any(String), |
There was a problem hiding this comment.
I'm with the robot on this one. Let's have specific color here, instead of expect.any.
|
The visual diff looks all reasonable except for the active shape Pie. It feels that this should show the active shape color, not the inactive shape. What do you think @2YH02 ? 
|
@PavelVanecek I agree. I reviewed the VR diffs and missed that part. |
|
Updated, @PavelVanecek Pie tooltip now uses |
| // color and fill are erased to keep 100% the identical behaviour to recharts 2.x - but there's nothing stopping us from returning them here. It's technically a breaking change. | ||
| fill: undefined, | ||
| // Preserve item-level color/fill from graphical items. | ||
| // @ts-expect-error we're assuming that item has color property |
There was a problem hiding this comment.
Let's parse it out, without assumptions and without ts expect error
There was a problem hiding this comment.
I changed this to parse array items from unknown before reading name/unit/dataKey/payload/color/fill
b49a59c
There was a problem hiding this comment.
The "as" keyword is doing exactly the same thing as ts-expect-error.
|
Does this fix #6957 too? |
| fill: string | undefined; | ||
| }; | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
And, you're not going to believe this, but the is keyword also disables the type system completely, same as @ts-expect-error and as. At least the runtime check is right this time.
There was a problem hiding this comment.
Thanks for the feedback!
Updated in 045367f.
|
Let's update the snapshots and then we can merge. |
|
https://63da8268a0da9970db6992aa-vqtbwaspqx.chromatic.com/?path=/story/api-chart-piechart--api this story shows inactive color still. |
|
Hm but that's a completely custom shape. Should we dig into that? Or should we assume that whoever got that far, can also modify the tooltip text color? |
|
I agree. Since this is a fully custom shape case, I think it’s reasonable to leave tooltip color to user control for now. In practice, users who customize charts this much usually customize tooltip content/styles too. If we later decide this should be auto-synced as well, I can follow up with a separate PR. |
Description
Fixes Pie tooltip/legend color sync when sectors are customized via
shape, by preserving per-sector color information in Pie tooltip payload flow.color/fillto Pie tooltip payload entries.chartType: 'pie'.color/fillfor Pie payload items in tooltip payload combiner (keeps existing behavior for non-Pie chart types).Related Issue
Fixes #6966
Fixes #6957
Motivation and Context
When Pie sectors are customized with
shape, visual sector colors can diverge from tooltip/legend color metadata.This change ensures Pie payload carries per-sector color data and that tooltip payload combining does not discard it for Pie, improving color consistency.
How Has This Been Tested?
Ran targeted tests locally:
npm run test -- test/polar/Pie/Pie.spec.tsxnpm run test -- test/state/selectors/pieSelectors.spec.tsxnpm run test -- test/component/Tooltip/defaultIndex.spec.tsxnpm run test -- test/component/Tooltip/itemSorter.spec.tsxnpm run test -- test/state/selectors/selectors.spec.tsxnpm run test -- test/component/Tooltip/tooltipEventType.spec.tsxnpm run test -- test/component/Tooltip/Tooltip.payload.spec.tsxnpm run check-types-storybooknpm run lintAlso added a Storybook example:
storybook/stories/Examples/Pie/PieColorSync.stories.tsxScreenshots (if appropriate):
pieChart.mov
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests