feat: pass axis padding info to custom tick components#6163
feat: pass axis padding info to custom tick components#6163ckifer merged 5 commits intorecharts:mainfrom
Conversation
|
Hi @ckifer 👋 I’ve opened a PR to address this issue. Would really appreciate your feedback when you get a chance! 🙏 Also, I wasn’t sure which Node.js version to use while setting up the project. Adding an Thanks in advance! |
|
Hey, thanks for the PR. Node version shouldn't matter too much as long as it's LTS but ack we can do that |
ckifer
left a comment
There was a problem hiding this comment.
PR looks good but your PR description says you have added test coverage when there are no unit tests. Can we add an assertion for each axis to the tests?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6163 +/- ##
==========================================
+ Coverage 96.50% 96.53% +0.02%
==========================================
Files 217 217
Lines 19825 19826 +1
Branches 4081 4084 +3
==========================================
+ Hits 19133 19139 +6
+ Misses 686 681 -5
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 332 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
Thanks for the review. I've added the tests to cover the changes now. |
ckifer
left a comment
There was a problem hiding this comment.
This test file is in a random folder. Can you remove the folder and just put the tests in XAxis.spec and YAxis.spec?
Don't let AI do it for you 😅
I think, now it should be okay! Feel free to drop your thoughts. |
| const axisSize = useAppSelector(state => selectYAxisSize(state, yAxisId)); | ||
| const position = useAppSelector(state => selectYAxisPosition(state, yAxisId)); | ||
| const cartesianTickItems = useAppSelector(state => selectTicksOfAxis(state, axisType, yAxisId, isPanorama)); | ||
| const axisSettings = useAppSelector(state => selectYAxisSettings(state, yAxisId)); |
There was a problem hiding this comment.
now that I think about it, why do you need to select this from axis settings when this is the component has access to padding in props?
you should be able to access padding with props.padding for both axes.
See line 153
There was a problem hiding this comment.
Yeah, Without it also works
| className={clsx(`recharts-${axisType} ${axisType}`, className)} | ||
| viewBox={viewBox} | ||
| ticks={cartesianTickItems} | ||
| padding={axisSettings.padding} |
There was a problem hiding this comment.
padding should already be passed with {...allOtherProps}
ckifer
left a comment
There was a problem hiding this comment.
thank you! The selection from state would've caused an extra re-render so this is much nicer
|
Hey @ckifer If everything looks good, Would you mind merging this PR ? Thanks |
|
Ah my bad I thought I did already |
Pass Axis Padding Info to Custom Tick Components
Description
This pull request implements a feature that allows custom tick components to receive axis padding information. Custom tick components can now access the
paddingprop to make informed positioning and styling decisions.Key Changes:
CartesianAxisinterface with optionalpaddingpropXAxisImplandYAxisImplto retrieve and pass padding from axis settings via Redux selectorsThe feature enables developers to create more sophisticated custom ticks that respond to padding configuration, eliminating the need to duplicate padding values or make assumptions about axis layout.
Related Issue
Closes #6130 - Custom tick component should get axis padding info
Motivation and Context
Previously, when using custom tick components with
XAxisorYAxis, developers faced several challenges:This change solves these problems by providing direct access to padding information, enabling:
How Has This Been Tested?
Comprehensive Test Suite
test/cartesian/XAxis.spec.tsx):test/cartesian/YAxis/YAxis.spec.tsx):Regression Testing
Storybook Stories
XAxis.stories.tsxandYAxis.stories.tsxCustomTickWithPadding: Demonstrates React element-based custom ticks with padding displayCustomTickFunction: Shows function-based custom ticks with dynamic positioning based on paddingTest Examples
Usage Examples
Basic XAxis Custom Tick
Advanced Use Cases
Responsive Tick Positioning
Function-based Custom Tick with Dynamic Positioning
Implementation Details
Type Definitions
XAxisPadding = { left?: number; right?: number } | 'gap' | 'no-gap'YAxisPadding = { top?: number; bottom?: number } | 'gap' | 'no-gap'Files Modified
src/cartesian/CartesianAxis.tsx- Added padding prop interface and tick integrationsrc/cartesian/XAxis.tsx- Added padding retrieval from axis settings via Redux selectorsrc/cartesian/YAxis.tsx- Added padding retrieval from axis settings via Redux selectortest/cartesian/XAxis.spec.tsx- Comprehensive unit test coverage for XAxis custom tick paddingtest/cartesian/YAxis/YAxis.spec.tsx- Comprehensive unit test coverage for YAxis custom tick paddingstorybook/stories/API/cartesian/XAxis.stories.tsx- Interactive examples and demosstorybook/stories/API/cartesian/YAxis.stories.tsx- Interactive examples and demosData Flow
paddingprop onXAxis/YAxiscartesianAxisSliceXAxisImpl/YAxisImplretrieve padding from settings usingselectXAxisSettings/selectYAxisSettingsselectorsCartesianAxiscomponent as a propCartesianAxisincludes padding in tick props when rendering custom tick componentsTypes of changes
Checklist:
Backward Compatibility
✅ 100% Backward Compatible
paddingprop doesn't break existing codePerformance Impact
Migration Guide
No migration required! Existing code continues to work unchanged.
To use the new feature:
The new feature is completely opt-in - custom tick components that don't use the
paddingprop will continue to work exactly as before.