-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Introduce the notion of series with positions #20461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Deploy preview: https://deploy-preview-20461--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging #20461 will not alter performanceComparing Summary
Footnotes |
bernardobelchior
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this approach looks cleaner, especially if we go with seriesLayout only containing the layout instead of the whole series.
One weird thing is that the layout won't be present for all series. I wonder if we should abstract getting an item's position somehow so that we can provide a getter and avoid calculating the whole layout for all charts. In a sankey chart, it would do the layout and find the item, returning its position, but in other charts it would call the axis scales to find its position.
| ...series, | ||
| series: { | ||
| [series.seriesOrder[0]]: { | ||
| ...series.series[series.seriesOrder[0]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the series in the seriesProcessor. Would it make sense to seriesPositions to only return the positions?
Otherwise we're spreading the series again unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I still return an object { sankeyLayout: (...) } instead of directly the layout.
My idea is to be able to add extra parameters latter if we need something
| >; | ||
| }; | ||
| series: DefaultizedPieSeriesType; | ||
| seriesComputedPosition: {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder we should call this seriesLayout to be shorter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed most of those with "layout"
JCQuintas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of moving the layout logic to the sankey selectors, but @alexfauquette pointed out that for the tooltip position a generic selector with the position would be better instead.
packages/x-charts/src/internals/plugins/corePlugins/useChartSeries/processSeries.ts
Outdated
Show resolved
Hide resolved
That could effectively be interesting. It would create a big selector depending on
It would basically replace hooks like the I'm doing the modification to add layout for the sankey, such that we could latter add support for other series if needed |
| seriesLayout: { | ||
| sankeyLayout: SankeyLayout; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefered this one instead of
seriesLayout: SankeyLayout;
yo ba able to add other layout if needed later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is internal, so we could have gone for the simpler solution, I think, but this doesn't look bad either.
| ids?: SeriesId | SeriesId[], | ||
| ) => { | ||
| if (!ids || (Array.isArray(ids) && ids.length === 0)) { | ||
| if (ids === undefined || (Array.isArray(ids) && ids.length === 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge case to support the case of a series with id set to an empty string.
684f49c to
a6225f4
Compare
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
| seriesLayout: { | ||
| sankeyLayout: SankeyLayout; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is internal, so we could have gone for the simpler solution, I think, but this doesn't look bad either.
| (Object.keys(processedSeries) as TSeriesType[]).forEach((type) => { | ||
| const processor = seriesConfig[type]?.seriesLayout; | ||
| if (processor !== undefined) { | ||
| const newValue = processor(processedSeries[type] as any, drawingArea); | ||
|
|
||
| if (newValue && newValue !== processedSeries[type]) { | ||
| processingDetected = true; | ||
| (seriesLayout as any)[type] = newValue; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my tests, we can remove as any by doing this:
| (Object.keys(processedSeries) as TSeriesType[]).forEach((type) => { | |
| const processor = seriesConfig[type]?.seriesLayout; | |
| if (processor !== undefined) { | |
| const newValue = processor(processedSeries[type] as any, drawingArea); | |
| if (newValue && newValue !== processedSeries[type]) { | |
| processingDetected = true; | |
| (seriesLayout as any)[type] = newValue; | |
| } | |
| } | |
| }); | |
| (Object.keys(processedSeries) as TSeriesType[]).forEach((type) => { | |
| const processor = seriesConfig[type]?.seriesLayout; | |
| const thisSeries = processedSeries[type]; | |
| if (processor !== undefined && thisSeries !== undefined) { | |
| const newValue = processor(thisSeries, drawingArea); | |
| if (newValue && newValue !== processedSeries[type]) { | |
| processingDetected = true; | |
| seriesLayout[type] = newValue; | |
| } | |
| } | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, That let me totice some other as was useless
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Alternative to #20437
I made alexfauquette#14 to illustrate how it will simplify future feature