Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds and exports a new useMargin hook to the public API, along with comprehensive documentation and storybook examples. The hook provides access to chart margin values defined by users in chart props, distinct from the useOffset hook which includes calculated space for axes, legends, and other elements.
- Export
useMarginhook from the main index and add comprehensive documentation - Update margin type definitions to distinguish between user-defined partial margins and internal complete margins
- Add storybook stories and visual components to demonstrate the hook functionality
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Exports the new useMargin hook to make it publicly available |
| src/context/chartLayoutContext.tsx | Updates useMargin hook implementation with proper documentation and typing |
| src/util/types.ts | Refines Margin type definitions to distinguish partial vs complete margins |
| src/state/layoutSlice.ts | Updates margin setter to handle partial margin objects with defaults |
| storybook/stories/API/hooks/useMargin.* | Adds complete documentation and interactive examples for the new hook |
| test/component/Legend.spec.tsx | Updates test to use proper chart context instead of isolated portal context |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| const [lastBoundingBox, updateBoundingBox] = useElementOffset([contextPayload]); | ||
| const chartWidth = useChartWidth(); | ||
| const chartHeight = useChartHeight(); | ||
| if (chartWidth == null || chartHeight == null) { |
There was a problem hiding this comment.
[nitpick] Consider using strict equality comparison (===) instead of loose equality (==) for null/undefined checks. Replace chartWidth == null with chartWidth === undefined and chartHeight == null with chartHeight === undefined for more explicit type checking.
| if (chartWidth == null || chartHeight == null) { | |
| if (chartWidth === undefined || chartHeight === undefined) { |
| if (chartWidth == null || chartHeight == null) { | ||
| return null; | ||
| } | ||
| const maxWidth = chartWidth - (margin.left || 0) - (margin.right || 0); |
There was a problem hiding this comment.
This line will throw a runtime error if chartWidth is undefined. The null check on line 140 should return early before this calculation, but the code continues to execute. Move this calculation inside the conditional block after the null check or ensure chartWidth is guaranteed to be defined at this point.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6224 +/- ##
==========================================
- Coverage 96.71% 96.67% -0.04%
==========================================
Files 221 221
Lines 19886 19923 +37
Branches 4103 4112 +9
==========================================
+ Hits 19233 19261 +28
- Misses 647 656 +9
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.9kB (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-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
|
I guess I don't really get this one. But doesn't hurt anything either I suppose |
Description
Export and document hook, refresh documentation a little, add story
Related Issue
Closes #5783
Motivation and Context
Margins are not exactly internal state - they are defined externally - but the hook will still be useful for shared components and other libraries.
Screenshots (if appropriate):
Types of changes
Checklist: