Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue with Label viewbox handling in polar charts by implementing a conditional logic fix to determine the appropriate viewbox based on label position. The main change adds logic to use cartesian viewbox for center-positioned labels and polar viewbox for other positions.
- Introduces conditional viewbox resolution in Label component based on position prop
- Adds comprehensive test coverage for Label component behavior in PieChart and LineChart contexts
- Updates PieChart storybook stories to use new data format and improved styling
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/component/Label.tsx | Implements viewbox resolution fix with conditional logic for polar vs cartesian charts |
| test/component/Label.spec.tsx | Adds extensive test coverage for Label component with different chart types and positions |
| storybook/stories/API/chart/PieChart.stories.tsx | Updates PieChart stories with new data format and enhanced donut chart styling |
| const resolvedViewBox = position === 'center' ? cartesianViewBox : (polarViewBox ?? cartesianViewBox); | ||
|
|
There was a problem hiding this comment.
[nitpick] The viewbox resolution logic is complex and could benefit from extraction into a helper function. Consider creating a resolveViewBox(position, polarViewBox, cartesianViewBox) function to improve readability and testability.
| const resolvedViewBox = position === 'center' ? cartesianViewBox : (polarViewBox ?? cartesianViewBox); | |
| const resolvedViewBox = resolveViewBox(position, polarViewBox, cartesianViewBox); |
| * I am not proud about this solution but it's a quick fix for https://github.com/recharts/recharts/issues/6030#issuecomment-3155352460. | ||
| * What we should really do is split Label into two components: CartesianLabel and PolarLabel and then handle their respective viewBoxes separately. | ||
| * Also other components should set its own viewBox in a context so that we can fix https://github.com/recharts/recharts/issues/6156 |
There was a problem hiding this comment.
[nitpick] The comment acknowledges this is a temporary solution but lacks specificity about why this approach was chosen and what makes it problematic. Consider documenting the specific issues with the current approach and timeline for the proper solution.
| * I am not proud about this solution but it's a quick fix for https://github.com/recharts/recharts/issues/6030#issuecomment-3155352460. | |
| * What we should really do is split Label into two components: CartesianLabel and PolarLabel and then handle their respective viewBoxes separately. | |
| * Also other components should set its own viewBox in a context so that we can fix https://github.com/recharts/recharts/issues/6156 | |
| * Temporary workaround: This logic mixes cartesian and polar viewBox handling in a single Label component. | |
| * This was chosen to quickly address https://github.com/recharts/recharts/issues/6030#issuecomment-3155352460, | |
| * but it introduces complexity and increases the risk of bugs, as the Label component now has to handle two fundamentally different coordinate systems. | |
| * This makes the code harder to maintain and reason about, and can lead to incorrect label rendering in edge cases. | |
| * The proper solution is to split Label into CartesianLabel and PolarLabel components, each handling their own viewBox logic. | |
| * Additionally, components should set their own viewBox in context to resolve https://github.com/recharts/recharts/issues/6156. | |
| * Plan: Refactoring to separate components is targeted for the next major release (v3.0), pending design discussion and community feedback. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6180 +/- ##
=======================================
Coverage 96.74% 96.74%
=======================================
Files 222 222
Lines 19985 19991 +6
Branches 4137 4138 +1
=======================================
+ Hits 19334 19340 +6
Misses 645 645
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 1.44kB (0.06%) ⬆️. 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:
|
| const viewBox = viewBoxFromProps || polarViewBox || viewBoxFromContext; | ||
| /* | ||
| * I am not proud about this solution but it's a quick fix for https://github.com/recharts/recharts/issues/6030#issuecomment-3155352460. | ||
| * What we should really do is split Label into two components: CartesianLabel and PolarLabel and then handle their respective viewBoxes separately. |
There was a problem hiding this comment.
Imo splitting them would make the API less straightforward.
But can't do it now anyways since breaking changes
Description
I am not too happy about this hack but it appears to be working fine for now.
Related Issue
#6030