Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes margin calculation issues in the Funnel component that were causing incorrect positioning and sizing. The fix addresses issues where the funnel was counting left margin twice, ignoring top margin, and incorrectly using top for X coordinates and left for Y coordinates.
Key changes:
- Corrected margin calculation logic in
getRealWidthHeightfunction - Fixed coordinate calculation to properly use
leftfor X andtopfor Y positioning - Improved documentation for the
getPercentValueutility function
Reviewed Changes
Copilot reviewed 6 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cartesian/Funnel.tsx | Fixed margin calculation logic and coordinate positioning |
| src/util/DataUtils.ts | Enhanced documentation for getPercentValue function |
| storybook/stories/API/chart/FunnelChart.stories.tsx | Removed hardcoded width from story example |
| test/cartesian/Funnel.spec.tsx | Updated test expectations for corrected positioning |
| test/cartesian/Funnel.animation.spec.tsx | Updated animation test expectations for corrected positioning |
| test/component/Tooltip/Tooltip.visibility.spec.tsx | Updated tooltip positioning test expectations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| type RealFunnelData = unknown; | ||
|
|
There was a problem hiding this comment.
Consider using a more specific type instead of unknown for RealFunnelData. While unknown is safer than any, a proper interface or type would improve type safety and code documentation.
| type RealFunnelData = unknown; | |
| interface FunnelDataItem { | |
| value?: number | string; | |
| name?: string; | |
| [key: string]: any; // Allow additional properties if needed | |
| } | |
| type RealFunnelData = FunnelDataItem[]; |
| const { width, height, left, top } = offset; | ||
|
|
||
| const realWidth: number = getPercentValue(customWidth, width, width); | ||
|
|
||
| return { | ||
| realWidth: realWidth - left - right - 50, | ||
| realHeight: realHeight - bottom - top, | ||
| offsetX: (width - realWidth) / 2, | ||
| offsetY: (height - realHeight) / 2, | ||
| realWidth, | ||
| realHeight: height, | ||
| offsetX: left, | ||
| offsetY: top, | ||
| }; |
There was a problem hiding this comment.
[nitpick] The realWidth calculation uses width as both the total value and default value for getPercentValue. Consider making this more explicit by passing a clear default value or adding a comment explaining this choice.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6453 +/- ##
==========================================
- Coverage 93.83% 93.83% -0.01%
==========================================
Files 417 417
Lines 38423 38422 -1
Branches 4502 4504 +2
==========================================
- Hits 36053 36052 -1
Misses 2353 2353
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 650 bytes (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
ckifer
left a comment
There was a problem hiding this comment.
LGTM but as mentioned might cause some friction depending on what consumers think. Overall, the before is pretty whack
Description
So I went and tried to fix #6427 and in the process I have discovered that, to my horror, Funnel is doing truly atrocious things with margins:
topfor X coordinateleftfor Y coordinateSo I'm fixing these. There will be visual diff, I consider this a bugfix.
Related Issue
#6427
Screenshots (if appropriate):
Before
Funnel is vaguely present within the plot area, but it's not matching it at all:
After
Funnel precisely matches margin and plot area:
Types of changes
Checklist: