Conversation
WalkthroughThe PR adds z-index layering support for bar backgrounds in both Cartesian and polar coordinate systems. It introduces a new default z-index value ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6536 +/- ##
==========================================
- Coverage 93.43% 93.42% -0.01%
==========================================
Files 436 436
Lines 39545 39566 +21
Branches 4599 4605 +6
==========================================
+ Hits 36950 36966 +16
- Misses 2579 2584 +5
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 1.99kB (0.08%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cartesian/Bar.tsx (1)
281-315: Add tests for background zIndex layering functionality.The new
BarBackgroundcomponent wrapped inZIndexLayerwith computedgetZIndex(backgroundFromProps)lacks test coverage. No existing tests cover either thegetZIndexfunction orZIndexLayerusage for backgrounds. The three existing background tests only validate basic rendering and props, not the zIndex layering behavior.Per coding guidelines: add appropriate tests when changing functionality and aim for 100% unit test coverage. Tests should verify that:
- Background renders within correct
ZIndexLayerwith appropriate zIndex valuegetZIndexcomputes correct zIndex based on background prop
🧹 Nitpick comments (1)
src/polar/RadialBar.tsx (1)
338-348: Consider extracting shared zIndex helper logic.The
getZIndexForRadialBarBackgroundfunction is nearly identical togetZIndexinsrc/cartesian/Bar.tsx(lines 249-254). Both extract a zIndex from a background object with the same fallback. Consider extracting this to a shared utility function to reduce duplication.Example shared utility:
// src/util/zIndexUtils.ts export function extractZIndexFromBackground( background: unknown, fallback: number ): number { if ( background != null && typeof background === 'object' && 'zIndex' in background && typeof background.zIndex === 'number' ) { return background.zIndex; } return fallback; }Then both components could use:
extractZIndexFromBackground(background, DefaultZIndexes.barBackground)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cartesian/Bar.tsx(3 hunks)src/polar/RadialBar.tsx(1 hunks)src/zindex/DefaultZIndexes.tsx(1 hunks)test/zindex/zIndexSelectors.spec.ts(4 hunks)www/src/docs/exampleComponents/BarChart/StackedBarChart.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const
Files:
src/zindex/DefaultZIndexes.tsxsrc/polar/RadialBar.tsxwww/src/docs/exampleComponents/BarChart/StackedBarChart.tsxtest/zindex/zIndexSelectors.spec.tssrc/cartesian/Bar.tsx
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users
Files:
src/zindex/DefaultZIndexes.tsxsrc/polar/RadialBar.tsxsrc/cartesian/Bar.tsx
www/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Use the www directory to add and commit examples for the documentation website (recharts.github.io)
Files:
www/src/docs/exampleComponents/BarChart/StackedBarChart.tsx
{test,www/test}/**
📄 CodeRabbit inference engine (DEVELOPING.md)
Place unit tests in the test directory; some tests may also live in www/test
Files:
test/zindex/zIndexSelectors.spec.ts
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
test/**/*.spec.{ts,tsx}: When writing new code, aim for 100% unit test code coverage
When adding or changing functionality, add appropriate tests (prefer unit tests; use RTL if rendering is involved)
Files:
test/zindex/zIndexSelectors.spec.ts
🧠 Learnings (7)
📚 Learning: 2025-10-25T07:36:02.211Z
Learnt from: CR
PR: recharts/recharts#0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.211Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
Applied to files:
www/src/docs/exampleComponents/BarChart/StackedBarChart.tsx
📚 Learning: 2025-10-25T07:35:14.559Z
Learnt from: CR
PR: recharts/recharts#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-25T07:35:14.559Z
Learning: Provide a reduced, reproducible test case (e.g., via CodeSandbox or fork of recharts.github.io) when reporting issues
Applied to files:
www/src/docs/exampleComponents/BarChart/StackedBarChart.tsx
📚 Learning: 2025-10-25T07:35:46.180Z
Learnt from: CR
PR: recharts/recharts#0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase
Applied to files:
test/zindex/zIndexSelectors.spec.ts
📚 Learning: 2025-10-25T07:35:46.180Z
Learnt from: CR
PR: recharts/recharts#0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests not using createSelectorTestCase, advance timers after renders with vi.runOnlyPendingTimers()
Applied to files:
test/zindex/zIndexSelectors.spec.ts
📚 Learning: 2025-10-25T07:35:14.559Z
Learnt from: CR
PR: recharts/recharts#0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-10-25T07:35:14.559Z
Learning: Applies to test/**/*.spec.{ts,tsx} : When adding or changing functionality, add appropriate tests (prefer unit tests; use RTL if rendering is involved)
Applied to files:
test/zindex/zIndexSelectors.spec.ts
📚 Learning: 2025-10-25T07:35:46.180Z
Learnt from: CR
PR: recharts/recharts#0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer using the createSelectorTestCase helper when writing or modifying tests
Applied to files:
test/zindex/zIndexSelectors.spec.ts
📚 Learning: 2025-10-25T07:35:46.180Z
Learnt from: CR
PR: recharts/recharts#0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid vi.runAllTimers() in tests to prevent infinite loops caused by chained scheduled timers
Applied to files:
test/zindex/zIndexSelectors.spec.ts
🧬 Code graph analysis (4)
src/polar/RadialBar.tsx (5)
src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-65)src/util/svgPropertiesNoEvents.ts (1)
svgPropertiesNoEventsFromUnknown(362-379)src/zindex/ZIndexLayer.tsx (1)
ZIndexLayer(44-103)src/util/RadialBarUtils.tsx (3)
RadialBarSectorProps(32-36)parseCornerRadius(7-13)RadialBarSector(38-40)src/util/types.ts (1)
adaptEventsOfChild(875-897)
www/src/docs/exampleComponents/BarChart/StackedBarChart.tsx (1)
src/cartesian/Bar.tsx (1)
Bar(891-891)
test/zindex/zIndexSelectors.spec.ts (1)
src/zindex/zIndexSelectors.ts (1)
selectAllRegisteredZIndexes(36-50)
src/cartesian/Bar.tsx (3)
src/util/types.ts (1)
ActiveShape(1037-1042)src/zindex/DefaultZIndexes.tsx (1)
DefaultZIndexes(4-65)src/zindex/ZIndexLayer.tsx (1)
ZIndexLayer(44-103)
🔇 Additional comments (5)
test/zindex/zIndexSelectors.spec.ts (1)
59-59: LGTM! Test expectations correctly updated for new default zIndex.All test expectations have been consistently updated to include the new
-50zIndex value forbarBackground. The value appears in the correct sorted position across all test scenarios.Also applies to: 68-68, 73-73, 79-79, 98-98, 112-112, 117-117, 125-125, 131-131, 139-139
www/src/docs/exampleComponents/BarChart/StackedBarChart.tsx (1)
68-69: LGTM! Example demonstrates the new background feature.The addition of the
backgroundprop to both stacked Bar components appropriately demonstrates the feature introduced by this PR, which addresses the z-index issue described in #2826.src/zindex/DefaultZIndexes.tsx (1)
9-13: LGTM! Well-documented default zIndex for bar backgrounds.The new
barBackgroundproperty with value-50is appropriately positioned in the z-index layering hierarchy and includes clear documentation about its purpose and usage.src/cartesian/Bar.tsx (1)
249-254: LGTM! Helper function correctly extracts zIndex with appropriate fallback.The
getZIndexhelper properly handles the various types thatActiveShapecan represent, using runtime type guards to safely extract thezIndexproperty when present, and falling back to the default value otherwise.src/polar/RadialBar.tsx (1)
338-348: LGTM! Helper function mirrors Bar implementation correctly.The
getZIndexForRadialBarBackgroundhelper appropriately handles the variousRadialBarBackgroundtypes and extracts zIndex with a sensible default fallback.
| return ( | ||
| <ZIndexLayer zIndex={getZIndexForRadialBarBackground(this.props.background)}> | ||
| {sectors.map((entry, i) => { | ||
| const { value, background, ...rest } = entry; | ||
|
|
||
| if (!background) { | ||
| return null; | ||
| } | ||
|
|
||
| const props: RadialBarSectorProps = { | ||
| cornerRadius: parseCornerRadius(cornerRadius), | ||
| ...rest, | ||
| // @ts-expect-error backgroundProps is contributing unknown props | ||
| fill: '#eee', | ||
| ...background, | ||
| ...backgroundProps, | ||
| ...adaptEventsOfChild(this.props, entry, i), | ||
| index: i, | ||
| className: clsx('recharts-radial-bar-background-sector', String(backgroundProps?.className)), | ||
| option: background, | ||
| isActive: false, | ||
| }; | ||
|
|
||
| return ( | ||
| <RadialBarSector | ||
| key={`background-${rest.cx}-${rest.cy}-${rest.innerRadius}-${rest.outerRadius}-${rest.startAngle}-${rest.endAngle}-${i}`} // eslint-disable-line react/no-array-index-key | ||
| {...props} | ||
| /> | ||
| ); | ||
| }); | ||
| return ( | ||
| <RadialBarSector | ||
| key={`background-${rest.cx}-${rest.cy}-${rest.innerRadius}-${rest.outerRadius}-${rest.startAngle}-${rest.endAngle}-${i}`} // eslint-disable-line react/no-array-index-key | ||
| {...props} | ||
| /> | ||
| ); | ||
| })} | ||
| </ZIndexLayer> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify test coverage for RadialBar background zIndex functionality.
The background rendering now correctly uses a ZIndexLayer for proper layering. However, per the coding guidelines, "when adding or changing functionality, add appropriate tests."
[As per coding guidelines]
Run the following script to check for existing tests covering RadialBar background with zIndex:
🏁 Script executed:
#!/bin/bash
# Search for tests related to RadialBar background and zIndex
rg -n -C3 --type=ts --type=tsx 'RadialBar.*background.*zIndex|background.*zIndex.*RadialBar' test/ -g '*.{spec,test}.{ts,tsx}'
# Search for RadialBar component tests
fd -e ts -e tsx 'RadialBar.*\.(spec|test)' test/Length of output: 172
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the test files for background-related tests
echo "=== Checking test/polar/RadialBar.spec.tsx for background tests ==="
grep -n -A5 -B2 "background" test/polar/RadialBar.spec.tsx | head -50
echo ""
echo "=== Checking test/polar/RadialBar.animation.spec.tsx for background tests ==="
grep -n -A5 -B2 "background" test/polar/RadialBar.animation.spec.tsx | head -50
echo ""
echo "=== Searching for ZIndexLayer tests in RadialBar tests ==="
grep -n -C3 "ZIndexLayer" test/polar/RadialBar*.spec.tsx test/chart/RadialBarChart*.spec.tsx 2>/dev/null
echo ""
echo "=== Searching for zIndex tests ==="
grep -n -C3 "zIndex" test/polar/RadialBar*.spec.tsx test/chart/RadialBarChart*.spec.tsx 2>/dev/null
Length of output: 1945
---
🏁 Script executed:
```shell
#!/bin/bash
# Get context of the background tests to understand what they're verifying
echo "=== Full test cases using background in RadialBar.spec.tsx ==="
grep -n -B10 "background:" test/polar/RadialBar.spec.tsx | grep -E "it\(|describe\(|background:" | head -20
echo ""
echo "=== Look for render/snapshot assertions in background tests ==="
grep -n -A20 "background:" test/polar/RadialBar.spec.tsx | grep -E "expect\(|toMatchSnapshot|render|screen" | head -15
echo ""
echo "=== Check RadialBar.tsx implementation for ZIndexLayer and background ==="
grep -n -A3 -B3 "ZIndexLayer.*background\|background.*ZIndexLayer" src/polar/RadialBar.tsx
echo ""
echo "=== Search for getZIndexForRadialBarBackground function ==="
grep -n "getZIndexForRadialBarBackground" src/polar/RadialBar.tsx
Length of output: 1483
---
🏁 Script executed:
```shell
#!/bin/bash
# View the getZIndexForRadialBarBackground function implementation
echo "=== getZIndexForRadialBarBackground function implementation ==="
sed -n '338,346p' src/polar/RadialBar.tsx
echo ""
echo "=== Check for snapshot tests in RadialBar.spec.tsx ==="
grep -n "toMatchSnapshot\|snapshot" test/polar/RadialBar.spec.tsx | head -10
echo ""
echo "=== Check the full test structure - how many test cases and what they do ==="
grep -n "it\(" test/polar/RadialBar.spec.tsx | wc -l
echo ""
echo "=== Find if any test specifically tests the background rendering output ==="
grep -n -A30 "should render sectors" test/polar/RadialBar.spec.tsx | head -40
Length of output: 3071
Add tests for RadialBar background zIndex functionality
Verification confirms a test coverage gap: while background rendering is tested via sector path assertions, the new ZIndexLayer wrapping and zIndex functionality are not. Tests should verify that:
- Background objects with a
zIndexproperty apply the correct z-index layer - Background objects without
zIndexuse the default value - The layering behavior correctly orders background sectors
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
Description
This is a problem in stacked charts so why not fix it now that we have the technology.
Related Issue
Fixes #2826
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes