fix: implement reverseStackOrder prop in v3 Redux architecture#6644
fix: implement reverseStackOrder prop in v3 Redux architecture#6644ckifer merged 1 commit intorecharts:mainfrom
Conversation
WalkthroughAdds a boolean Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Chart Component
participant Redux as Redux State
participant Selectors as selectStackGroups
participant Combiner as combineStackGroups
participant Renderer as ReportChart / Renderer
UI->>Redux: updateOptions({ reverseStackOrder })
UI->>Redux: read rootProps (incl. reverseStackOrder)
Redux->>Selectors: selectStackGroups(state, ...)
Selectors->>Combiner: (displayedData, items, stackOffsetType, reverseStackOrder)
alt reverseStackOrder = true
Note right of Combiner `#f8d7da`: Reverse graphicalItems before computing dataKeys
Combiner->>Renderer: StackGroup[] (reversed order)
else reverseStackOrder = false
Note right of Combiner `#d1e7dd`: Keep original graphical order
Combiner->>Renderer: StackGroup[] (original order)
end
Renderer->>UI: rendered chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code graph analysis (2)src/state/selectors/tooltipSelectors.ts (1)
test-vr/tests/BarChart.reverseStackOrder.spec-vr.tsx (1)
🔇 Additional comments (6)
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: 0
🧹 Nitpick comments (1)
src/state/selectors/axisSelectors.ts (1)
612-647: Implementation looks correct.The reversal logic properly creates a shallow copy before reversing, ensuring no mutation of the original array. The
orderedGraphicalItemsvariable is used consistently for both dataKeys derivation and the final result.Consider adding JSDoc documentation for the
reverseStackOrderparameter to help future maintainers understand its purpose:+/** + * Combines stack groups from graphical items. + * @param displayedData - The displayed stacked data + * @param items - Stacked graphical items to group + * @param stackOffsetType - The stack offset type (e.g., 'expand', 'silhouette') + * @param reverseStackOrder - When true, reverses the order of items within each stack + * @returns All stack groups indexed by stack ID + */ export const combineStackGroups = ( displayedData: DisplayedStackedData, items: ReadonlyArray<DefinitelyStackedGraphicalItem>, stackOffsetType: StackOffsetType, reverseStackOrder: boolean, ): AllStackGroups => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/chart/CartesianChart.tsx(1 hunks)src/chart/PolarChart.tsx(1 hunks)src/state/rootPropsSlice.ts(3 hunks)src/state/selectors/axisSelectors.ts(4 hunks)src/state/selectors/radialBarSelectors.ts(2 hunks)src/state/selectors/rootPropsSelectors.ts(1 hunks)src/state/selectors/tooltipSelectors.ts(2 hunks)storybook/stories/API/chart/BarChart.stories.tsx(1 hunks)test/state/selectors/selectStackGroups.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/state/selectors/rootPropsSelectors.ts (1)
src/state/store.ts (1)
RechartsRootState(94-94)
test/state/selectors/selectStackGroups.spec.tsx (5)
src/context/PanoramaContext.tsx (1)
useIsPanorama(6-6)test/helper/selectorTestHelpers.tsx (1)
useAppSelectorWithStableTest(86-93)src/state/selectors/axisSelectors.ts (1)
selectStackGroups(654-662)test/_data.ts (1)
PageData(4-11)test/helper/expectLastCalledWith.ts (1)
expectLastCalledWith(14-34)
src/state/selectors/radialBarSelectors.ts (1)
src/state/selectors/rootPropsSelectors.ts (2)
selectStackOffsetType(11-11)selectReverseStackOrder(12-12)
src/state/selectors/axisSelectors.ts (3)
src/util/stacks/getStackSeriesIdentifier.ts (1)
getStackSeriesIdentifier(14-18)src/util/ChartUtils.ts (1)
getStackedData(427-441)src/state/selectors/rootPropsSelectors.ts (2)
selectStackOffsetType(11-11)selectReverseStackOrder(12-12)
src/state/selectors/tooltipSelectors.ts (1)
src/state/selectors/rootPropsSelectors.ts (2)
selectStackOffsetType(11-11)selectReverseStackOrder(12-12)
🔇 Additional comments (14)
src/chart/CartesianChart.tsx (1)
77-77: LGTM! Prop forwarding is consistent.The
reverseStackOrderprop is correctly forwarded toReportChartProps, following the same pattern as other chart options.src/state/selectors/radialBarSelectors.ts (2)
31-31: LGTM! Selector import is correct.The
selectReverseStackOrderimport follows the established pattern for root props selectors.
352-354: LGTM! Selector dependency correctly wired.The
selectReverseStackOrderis properly added to theselectStackGroupsdependency list, enablingcombineStackGroupsto access the reverse stacking flag. This change is consistent with similar updates inaxisSelectors.tsandtooltipSelectors.ts.storybook/stories/API/chart/BarChart.stories.tsx (1)
101-101: LGTM! Storybook control properly configured.The
reverseStackOrdercontrol is appropriately added to the Stacked story with the correct default value, enabling manual testing of the feature.src/state/selectors/tooltipSelectors.ts (2)
54-54: LGTM! Import correctly added.The
selectReverseStackOrderimport is properly placed with other root props selectors.
173-175: LGTM! Tooltip stack groups will respect reverse order.The
selectReverseStackOrderis correctly added toselectTooltipStackGroupsdependencies, ensuring tooltips display stacked data in the correct order when the flag is enabled.test/state/selectors/selectStackGroups.spec.tsx (1)
319-464: LGTM! Comprehensive test coverage for reverseStackOrder.The test suite thoroughly validates both
reverseStackOrder={true}andreverseStackOrder={false}scenarios, verifying that:
- Graphical items order is correctly reversed/maintained
- Stacked data order matches the graphical items order
- The expected stacked values are accurate
The tests follow established patterns from the existing test suite and provide solid coverage for the new feature.
src/state/selectors/rootPropsSelectors.ts (1)
12-12: LGTM! Selector correctly implemented.The
selectReverseStackOrderselector follows the established pattern for root props selectors and has the correct type signature.src/chart/PolarChart.tsx (1)
94-94: LGTM! Prop forwarding is consistent with CartesianChart.The
reverseStackOrderprop is correctly forwarded toReportChartProps, maintaining consistency with the CartesianChart implementation.src/state/rootPropsSlice.ts (3)
28-32: LGTM! Clear documentation for the new field.The JSDoc comment clearly explains the
reverseStackOrderbehavior, helpfully clarifying that it affects SVG layering rather than x position.
46-46: LGTM! Initial state correctly set.The default value of
falseis appropriate, maintaining backward compatibility with existing behavior.
64-64: LGTM! Reducer correctly propagates the value.The
updateOptionsreducer properly assigns thereverseStackOrdervalue, following the established pattern for other updatable chart options.src/state/selectors/axisSelectors.ts (2)
75-80: LGTM!The import of
selectReverseStackOrderis correctly structured and follows the existing pattern for similar selectors.
654-662: LGTM!The selector integration correctly adds
selectReverseStackOrderas an input dependency and passes it tocombineStackGroups. The wiring is clean and follows the existing selector pattern.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6644 +/- ##
=======================================
Coverage 94.17% 94.17%
=======================================
Files 494 494
Lines 41666 41673 +7
Branches 4823 4824 +1
=======================================
+ Hits 39240 39247 +7
Misses 2421 2421
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @j-shimizu111 . Can you please add a new VR test too? There are examples in the tests-vr directory |
975faa4 to
48b39f1
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
/update-snapshots |
|
@PavelVanecek |
|
You can generate the screenshots with |
The reverseStackOrder prop was defined in types but not implemented in the v3 Redux-based architecture, causing it to have no effect. This change: - Adds reverseStackOrder to Redux state (UpdatableChartOptions) - Passes reverseStackOrder through ReportChartProps in CartesianChart and PolarChart - Implements stack reversal logic in combineStackGroups selector - Adds comprehensive tests for reverseStackOrder behavior - Updates Storybook story to expose reverseStackOrder control Fixes the regression from v2.15.4 where reverseStackOrder worked correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
48b39f1 to
c3ae91a
Compare
|
Thanks! I've added the VR tests and generated all snapshots locally with Docker as you suggested. All changes have been committed and pushed. The CI workflow is waiting for approval to run. Could you please approve it when you have a chance? |
Bundle ReportChanges will increase total bundle size by 660 bytes (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
Description
The
reverseStackOrderprop was defined in types but not implemented in the v3 Redux-based architecture, causing it to have no effect. This PR restores the functionality that worked correctly in v2.15.4.Changes
This PR implements
reverseStackOrderthroughout the Redux architecture:reverseStackOrdertoUpdatableChartOptionsinrootPropsSlice.tsCartesianChartandPolarChartto passreverseStackOrderthroughReportChartPropscombineStackGroupsinaxisSelectors.tsto reverse stack order when enabledtooltipSelectors.tsandradialBarSelectors.tsto includereverseStackOrderparameterreverseStackOrdercontrol to BarChart Stacked story for easy testingreverseStackOrder={true}andreverseStackOrder={false}casesVisual Demonstration
The fix can be tested in Storybook by navigating to API > chart > BarChart > Stacked and toggling the
reverseStackOrdercontrol. When enabled, the stacked bars render in reverse order (affecting SVG layering).Before (v3 without fix)
2025-11-17.13.16.40.mov
After (v3 with fix)
2025-11-17.13.45.16.mov
Types of changes
Checklist
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
New Features
Documentation
Tests