[charts-pro] Allow customizing scatter preview marker size #18726
Conversation
| @@ -36,6 +36,10 @@ const seriesProcessor: SeriesProcessor<'scatter'> = ({ series, seriesOrder }, da | |||
| labelMarkType: 'circle' as const, | |||
| markerSize: 4, | |||
| ...seriesData, | |||
| preview: { | |||
There was a problem hiding this comment.
Not sure where to document this feature. It's scatter-only, so it doesn't really make sense to document it in the zoom and pan section, but the scatter chart doesn't contain any information about zoom and preview 🤔
Any suggestions?
There was a problem hiding this comment.
Zoom&pan page I guess. Doesn't make sense to add it to scatter page I think
|
Deploy preview: https://deploy-preview-18726--material-ui-x.netlify.app/ Updated pages: Bundle size reportTotal Size Change: ▼-34.9KB(-0.26%) - Total Gzip Change: ▼-6.54KB(-0.16%) Show details for 100 more bundles (22 more not shown)@mui/x-charts-pro/ChartZoomSlider parsed: 🔺+54B(+0.05%) gzip: 🔺+16B(+0.05%) |
CodSpeed Performance ReportMerging #18726 will degrade performances by 11.74%Comparing Summary
Benchmarks breakdown
|
8ff3f2f to
b5ccb50
Compare
| @@ -105,7 +103,7 @@ function ScatterPreviewItems(props: ScatterPreviewItemsProps) { | |||
| x={dataPoint.x} | |||
| y={dataPoint.y} | |||
| seriesId={series.id} | |||
| size={DEFAULT_MARKER_SIZE} | |||
| size={series.preview.markerSize} | |||
There was a problem hiding this comment.
| size={series.preview.markerSize} | |
| size={series.preview.markerSize ?? 1} |
| preview: { | ||
| markerSize: 1, | ||
| ...seriesData?.preview, | ||
| }, |
There was a problem hiding this comment.
What about just using the default when needed
| preview: { | |
| markerSize: 1, | |
| ...seriesData?.preview, | |
| }, |
There was a problem hiding this comment.
I'd rather have a single place where we define the defaults rather than having the defaults spread across the consumers of these fields.
I think it's easier if the components consuming the state don't need to know the default values.
What's your concern with doing this in the series processor?
There was a problem hiding this comment.
It's just that we create a new preview object for little reasons
Adding default values to a configuration at the root of the processing pipeline is useful if it's used at multiple places.
But rendering options like markerSize are only used in one place with no side effects.
It should only appear if the user creates their own preview rendering. And in that case, I doubt they care about our default values
But clearly not a big deal
There was a problem hiding this comment.
So your concern is mostly performance?
I agree that it's unnecessary, but it's also likely negligible since this only runs once per series, and it's unlikely there will be many series.
Nevertheless, I'll check if the series processor has any impact in large charts.
Related to #18267 (comment).
Allow customizing scatter preview marker size.
Screen.Recording.2025-07-07.at.16.50.18.mov