[charts-pro] Add chart zoom preview #18267
Conversation
|
Deploy preview: https://deploy-preview-18267--material-ui-x.netlify.app/ Updated pages: Bundle size reportTotal Size Change: 🔺+104KB(+0.78%) - Total Gzip Change: 🔺+33.6KB(+0.83%) Show details for 100 more bundles (22 more not shown)@mui/x-charts-pro/ChartZoomSlider parsed: 🔺+37.2KB(+54.04%) gzip: 🔺+11.3KB(+46.48%) |
CodSpeed Performance ReportMerging #18267 will not alter performanceComparing Summary
|
| (_, params: { drawingArea: ChartDrawingArea; zoomMap: Map<AxisId, ZoomData> | undefined }) => | ||
| params, | ||
| ], | ||
| (axis, formattedSeries, seriesConfig, zoomOptions, getFilters, { drawingArea, zoomMap }) => |
There was a problem hiding this comment.
| (_, params: { drawingArea: ChartDrawingArea; zoomMap: Map<AxisId, ZoomData> | undefined }) => | |
| params, | |
| ], | |
| (axis, formattedSeries, seriesConfig, zoomOptions, getFilters, { drawingArea, zoomMap }) => | |
| (_, drawingArea: ChartDrawingArea) => drawingArea, | |
| (_, __, zoomMap: Map<AxisId, ZoomData> | undefined ) => zoomMap, | |
| ], | |
| (axis, formattedSeries, seriesConfig, zoomOptions, getFilters, drawingArea, zoomMap) => |
This with the following modification, and a memorization of drawingArea should avoid re-rendering the review axes
- const { axis: xAxis, axisIds: xAxisIds } = useSelector(store, selectorChartComputedXAxes, {
- drawingArea,
- zoomMap: undefined,
- });
+ const { axis: xAxis, axisIds: xAxisIds } = useSelector(store, selectorChartComputedXAxes, drawingArea, undefined);Since the zoom preview should not be zoomed itself, the are the zoomOptions, and getFilters needed?
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
bd6ef3d to
1ce50ee
Compare
f388172 to
59a0d76
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
da57a03 to
44fdfb8
Compare
fe859f5 to
ce0193b
Compare
719635c to
cc097f3
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
3316cb0 to
3d131d3
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
28df25c to
eb0e36d
Compare
56dcdef to
6a38fa1
Compare
| id: 'x', | ||
| data: new Array(101).fill(null).map((_, i) => i), | ||
| label: 'Age', | ||
| valueFormatter: (v) => (v === 100 ? '100+' : `${v}`), |
There was a problem hiding this comment.
| valueFormatter: (v) => (v === 100 ? '100+' : `${v}`), | |
| valueFormatter: (v) => (v === 100 ? '100+' : v.round().toString()), |
There was a problem hiding this comment.
Why should we round the age?
There was a problem hiding this comment.
It's because of the zoom tooltip
| d={d} | ||
| color={color} | ||
| gradientId={gradientId} | ||
| skipAnimation |
There was a problem hiding this comment.
I don't know why but animation is still visible
Capture.video.du.2025-07-03.09-42-48.mp4
There was a problem hiding this comment.
Ah, figured it out. It's because we're rendering two clipPath with the same ID, so it's probably using the first one.
There was a problem hiding this comment.
Fixed it by creating a slimmed down version of AreaElement and LineElement for the preview.
There was a problem hiding this comment.
Ah, figured it out. It's because we're rendering two clipPath with the same ID, so it's probably using the first one.
NIce catch. Those duplicated ids make me crazy when I've to debug them 😅
There was a problem hiding this comment.
The browser could at least warn if two elements have the same id 😆
There was a problem hiding this comment.
I wonder if we should start using useId. Do we expect the clip path IDs to be stable? If we were to use useId, then we wouldn't have this problem.
There was a problem hiding this comment.
The initial idea was to use the useId to generate a unique id per chart. Then deriving all ids from this one.
It's not super useful, so fully open for other solution on this aspect
There was a problem hiding this comment.
deriving all ids from this one
Are we using it for anything?
If users can't set the ID, then it isn't reliable, so I'm failing to see the benefit
There was a problem hiding this comment.
I don't think we use it. The only usecases I found in the docs are in the line Demo where we use useChartId() mostly because it's a convenient way to generate ids in the demo without relying on unstable imports.
There was a problem hiding this comment.
without relying on unstable imports
Which unstable imports are you referring to?
Do you mean this? I've just updated this in the last commit. Had some changes that I forgot to revert. |
47a85d7 to
1ffece1
Compare
98806b1 to
053fc71
Compare
Part of #15383.
Screen.Recording.2025-06-27.at.09.25.44.mov
Border radius in bar charts
For now, bar chart preview does not show a border radius. Zoom only makes sense for a lot of data; in that case, the border radius is barely visible.
This can be implemented if we feel there's a use case where it would make sense.
Handling slots
At the moment, we aren't handling slots. Slots are provided by prop to
BarElementandLineElement, and we aren't providing them, so there is no way to customize the preview rendering using slots.It's still possible to customize the preview by also personalizing the main chart, as those props will apply to both the main and the preview charts.
Still not sure how to handle slots for the preview: should we use specific slots or the same as the main chart? Should we allow customizing at all?
Animation
Decided not to animate the preview. We can do it later, if needed.