[charts] Add a series processor using the drawing area #20437
Conversation
alexfauquette
requested review from
JCQuintas and
bernardobelchior
as code owners
alexfauquette
added
type: enhancement
alexfauquette
changed the title
|
Deploy preview: https://deploy-preview-20437--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging #20437 will not alter performanceComparing Summary
Footnotes |
| * This selector computes the processed series on-demand from the defaultized series. | ||
| * @returns {ProcessedSeries} The processed series. | ||
| */ | ||
| export const selectorChartSeriesProcessed = createSelectorMemoized( |
There was a problem hiding this comment.
If we are using the same selector, why not make the regular series processor use the drawing area?
There was a problem hiding this comment.
With this approach if the drawing area is modified, but no series uses it the selectors is an identity function.
Not sure it's usefull. But if some selector uses the processed series and not the drawing area they should be able to avoid a recomputation. Maybe bernarod's work on the scatter zoom performance can benefit from it. Not sure
| seriesProcessor: SeriesProcessorWithoutDimensions<TSeriesType>; | ||
| colorProcessor: ColorProcessor<TSeriesType>; | ||
| legendGetter: LegendGetter<TSeriesType>; | ||
| tooltipGetter: TooltipGetter<TSeriesType>; | ||
| tooltipItemPositionGetter?: TooltipItemPositionGetter<TSeriesType>; | ||
| getSeriesWithDefaultValues: GetSeriesWithDefaultValues<TSeriesType>; | ||
| seriesProcessorWithDrawingArea?: SeriesProcessor<TSeriesType>; |
There was a problem hiding this comment.
Shouldn't these types names be inverted?
Like
seriesProcessor -> SeriesProcessor<TSeriesType>
seriesProcessorWithDrawingArea -> SeriesProcessorWithDrawingArea<TSeriesType>or
seriesProcessorWithoutDrawingArea -> SeriesProcessorWithoutDrawingArea<TSeriesType>
seriesProcessor -> SeriesProcessor<TSeriesType>There was a problem hiding this comment.
I will got that way
seriesProcessorWithoutDrawingArea -> SeriesProcessorWithoutDrawingArea<TSeriesType>
seriesProcessor -> SeriesProcessor<TSeriesType>The underlying reason I did that is the type renaming. Lot of stuff rely on the SeriesProcessor so I kept it as it to avoid doubling the number of files with dumb modification and having WithDrawingArea every where in the codebase
There was a problem hiding this comment.
Yeah, let's do the renaming in a follow-up PR
There was a problem hiding this comment.
JC renaming proposal is not that impacting. I can do it now
|
This change seems to be made because the Sankey provides layout information in its data in the It doesn't make sense to provide the layout information here because it isn't usable. For example, if the sankey resizes, the layout will be outdated, but the user won't be informed of the resize. Or if the sankey data changes, causing a re-layout, then the layout info will also be outdated. If a user wants to position something relative to a node maybe we should provide a way to do it that is more inline with React, e.g., by using a component that re-renders when the layout changes. If we remove the layout information from the identifier, we don't need this change, right? Would it make sense to remove it, then? CC @JCQuintas as you might have more insights |
| if (!processingDetected) { | ||
| return processedSeries as ProcessedSeries<TSeriesType>; | ||
| } |
There was a problem hiding this comment.
Is processingDetected just to avoid breaking selectors' cache when processing results in the same value?
There was a problem hiding this comment.
Yes, that way either not providing seriesProcessor() or having seriesProcessor=(x)=>x will be transparent
| seriesProcessor: SeriesProcessorWithoutDimensions<TSeriesType>; | ||
| colorProcessor: ColorProcessor<TSeriesType>; | ||
| legendGetter: LegendGetter<TSeriesType>; | ||
| tooltipGetter: TooltipGetter<TSeriesType>; | ||
| tooltipItemPositionGetter?: TooltipItemPositionGetter<TSeriesType>; | ||
| getSeriesWithDefaultValues: GetSeriesWithDefaultValues<TSeriesType>; | ||
| seriesProcessorWithDrawingArea?: SeriesProcessor<TSeriesType>; |
There was a problem hiding this comment.
Yeah, let's do the renaming in a follow-up PR
| series: DefaultizedBarSeriesType; | ||
| /** | ||
| * Additional data computed from the series plus drawing area. | ||
| * Usefull for special charts like sankey where the series data is not sufficient to draw the series. |
There was a problem hiding this comment.
| * Usefull for special charts like sankey where the series data is not sufficient to draw the series. | |
| * Useful for special charts like sankey where the series data is not sufficient to draw the series. |
|
@bernardobelchior About the layout, I agree, the position is not interesting to provide to users. But some other aspect are. In particular the sankey layout provides a nice way to navigate the graph. If you click on a node, the |
Right, but for that we don't need the drawing area, do we? That's only derived from the data, no need to add the drawing area as a dependency, or am I missing something? |
Yes but that's done all at once in the |
|
Isn't it easier to replicate that logic just for the sankey chart than adding a As a shortcut, we can use a dummy drawing area just to obtain the same data structure and hide the layout information. Then, if we want to, we can replace that with our implementation which would probably be faster since we don't need to lay out the nodes and links. Obviously, we'd still keep d3-sankey for layout. Additionally, we could make it so that What do you think? |
For me this solution is more future proof. For example it solves the issue about node anchoring for sankey For now most of our charts are a mix between an "axis management" + "series management" And the tooltip, highlight are designed from them. Based on a With special charts like the sankey or the tree map, having a special piece of code to handle coordinates based on the drawing area does not make that much sense since it will not be shared with others The proposal is to have
The adventage for us is to keep the tooltip/highlight management consistent with the other series |
|
I'm not sure I got your comment 😅
So the idea is that for sankey the In other charts, Since a Sankey chart doesn't have axis, a I think exposing a Plus, we could remove the dependency of series on the drawing area. This can provide other performance benefits: components that only depend on the data/series wouldn't re-render on drawing area changes, e.g., tooltip contents. The layout is derived from the data + drawing area, so exposing it separately will probably be a good idea to reduce unnecessary computation. What do you think? |
|
What bother me with a dedicated hook is that it will create edge case management. Because for now the tootlip content or tooltip position are simple:
If we have a dedicated
For me it's not that much of a deal. The current modification implies two selectors 
We have two different cases: The series types uses the additional selector (basically the sankey. Potentially the pie chart) or the series type does not use it (i.e. bar, line, scatter) We can ignore the second case, because the second selector is the identity function. so if the drawing area get modified, it will just loop other bar, line, and scatter to make sure their config does not contain a processor using the drawin area For the sankey, we could effectively have a small improvement by using the result of the first selectors when layout is not needed. For example in tooltip/legend. What about the following:
Here I talk about sankey, but it might be interesting to do the same stuff to compute pie charts slices only once for all. Currenlty it's done in PieArcPlot, PieArcLabelPlot, and could simplify the |
But won't we need a separate case regardless? Even if the layout is part of the Maybe I'm missing something 😅
Sounds good 👍
What do you mean by this? I suppose it's adding a |
I think you miss the Such that adding an
For the function useSankeyLayout() {
const store = useStore()
const series = useSelector(store, selectorChartSeriesProcessedWithPositions)
return series.sankey.series[series.sankey.orderedIds[0]].sankeyLayout
}For the point 4. I'm especially thinking about the tooltip item position export const selectorChartsTooltipItemPosition = createSelector(
selectorChartsTooltipItem,
selectorChartDrawingArea,
selectorChartSeriesConfig,
selectorChartXAxis,
selectorChartYAxis,
- selectorChartSeriesProcessed,
+ selectorChartSeriesProcessedWithPositions,
(_, placement?: 'top' | 'bottom' | 'left' | 'right') => placement,
function selectorChartsTooltipItemPosition<T extends ChartSeriesType>( |
4 participants
Before
The sankey introduced a notion of series item with data.
The issue is that data are computed in the
SankeyPlotcomponent.Issues
If you want to controled tooltip, you can't expect to user to provide the additional data. They shoudl be able to say "Place tooltip on the node with id xxx"
So we need to be able to derive the
ItemIdentifierWithDatafrom theItemIdentifier.It's also usefull to compute the item position only once. For example for anchored tooltip, it would be nice to get node/links coordinates
After
Modifications
I added in the config an object that defines additional properties that can be added when the processor has access to the drawing area.
It's done such that if none of the series need that the object references stay unchanged
Potential use cases
This could also be usefull for series like the pie chart to compute the center