ResponsiveContainer is built-in to all charts#6388
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates responsive container functionality directly into all chart components, eliminating the need to wrap charts with ResponsiveContainer in most cases. Charts without explicit dimensions now default to width=100% and height=100%, preventing invisible charts.
Key changes:
- Built-in responsive behavior in all chart types (CartesianChart, PolarChart, Sankey, SunburstChart, Treemap)
- New aspectRatio prop added to chart components
- Type improvements with Percent types and proper ReactNode usage
- Enhanced test coverage for dimension calculations
Reviewed Changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/chart/*.tsx | Wraps chart components with ResponsiveContainer and adds aspectRatio prop |
| src/util/types.ts | Updates chart prop types to support Percent and adds aspectRatio |
| src/component/ResponsiveContainer.tsx | Refactors to avoid unnecessary container divs for fixed dimensions |
| test/**/*.spec.tsx | Updates tests to provide explicit dimensions where needed |
| storybook/stories/Examples/ScatterChart.stories.tsx | Removes redundant ResponsiveContainer wrappers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6388 +/- ##
==========================================
+ Coverage 62.04% 62.08% +0.03%
==========================================
Files 412 412
Lines 37883 37917 +34
Branches 4338 4337 -1
==========================================
+ Hits 23505 23539 +34
Misses 14269 14269
Partials 109 109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 7.46kB (0.3%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
ckifer
left a comment
There was a problem hiding this comment.
My only qualm with this PR is that I think this makes SSR even more broken than it already is?
Pre-3.0 people would SSR charts without responsive container because otherwise resize observer wouldn't be available.
I suppose we can add a conditional to just return children at some point easily enough
| * as whatever size we detect won't be helping anyway. | ||
| */ | ||
| return ( | ||
| <ResponsiveContainerContextProvider width={calculatedWidth} height={calculatedHeight}> |
There was a problem hiding this comment.
I did add this shortcut @ckifer - in case we are able to calculate specific size from the given arguments then the helper div doesn't render at all.
I haven't tested SSR though, and I am not sure what is the problem in SSR in the first place, so can't tell if this makes it better or worse.
There was a problem hiding this comment.
Yeah just need to make sure the things before this don't crash things out and actually make it here I guess
| * If you define both width and height on the treemap, this prop will be ignored for the main chart, | ||
| * but will still be used for the rectangles. | ||
| */ | ||
| aspectRatio?: number; |
There was a problem hiding this comment.
Also callout on Treemap.aspectRatio - this prop was there before, but only applied on the individual nodes. I think it makes sense to have it apply on the chart as a whole as well.
|
We probably should update the docs when this one goes out |
|
storybook test timeout hmmm, will re-run once |
Description
I never really understood why does this need to be its own element so let's have these props directly on the main chart shall we.
This also means that now charts without any dimensions at all will default to 'width=100% height=100%'. Before this PR, chart without dimensions is just quietly invisible which I think is a disaster. In a way this is a change of default behaviour - but I prefer to think of it as a bugfix.
The ResponsiveContainer itself is not as important as before but it still has some use I suppose - for example in the Bubble story it allows setting the dimensions from one place. Although that could be a variable too.