[charts] Experiment with plugins for series+axes #15865

alexfauquette · 2024-12-12T15:32:03Z

This PR is an attempt to migrate the axes, series and zoom to the plugin framework.

Modifications

Renaming that could be extracted

ZoomOptions -> ZoomOption: Even if we have multiple options, it's too easy to mix the zoom option of an axis and the object that map axes ids to their options
SeriesFormatterResult -> SeriesProcessorResult
SeriesFormatter -> SeriesProcessor
SeriesFormatterParams -> SeriesProcessorParams
ExtremumGetter -> CartesianExtremumGetter
ChartsPlugin -> ChartSeriesTypeConfig the definition of extremums, cologetter, ... for a series type

Logic

Instance

The isPointInside got moved from the drawing area to the instance. It's our first instance method 🎉

Renaming plugins

The plugins props which was an array of configuration objects, defining for each series how to compute the extremums, preprocess the data, ... is now an object called seriesConfig. That's only sementic modification

- plugins={[ barConfig, lineConfig ]}
+ seriesConfig={{ bar: barConfig, line: lineConfig }}

Selector usage

The data modification is now done in selectors. We have 3 states: series, cartesianAxes, and zoom which describe the state of the chart.

One issue with this strategy is that useXAxis() is defined in the MIT package. And if we want to have the BarPlot supporting zoom out of the box for pro users, we need to have the zoom selectors that get zoom data.

The workaround is to put types and selectors for zoom in the MIT package. But populate and manage the state in the pro package.

Zoom API

The zoom seems to be working ok. The new API proposed it the following one:

onZoomChange now provide the new zoom value in its argument.
zoom props is replaced by initialZoom which is only used to initialize the zoom.
The apiRef get a method setZoomData that use can call to set the zoom data. Maybe setZoom would be a better name

mui-bot · 2024-12-12T15:36:00Z

Deploy preview: https://deploy-preview-15865--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against b38dc27

JCQuintas · 2024-12-16T09:58:14Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/useChartProZoom.ts

+  useEnhancedEffect(() => {
+    store.update((prevState) => ({
+      ...prevState,
+      zoom: {
+        ...prevState.zoom,
+        zoomData,
+      },
+    }));
+  }, [store, zoomData]);


Shouldn't this be inside the setZoomDataCallback instead? 🤔

github-actions · 2024-12-17T12:14:08Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

codspeed-hq · 2024-12-17T23:48:55Z

CodSpeed Performance Report

Merging #15865 will improve performances by 9.86%

_{Comparing alexfauquette:all-pluggin-way (12fadbe) with master (d9e6746)}

Summary

⚡ 1 improvements
✅ 5 untouched benchmarks

Benchmarks breakdown

	Benchmark	`master`	`alexfauquette:all-pluggin-way`	Change
⚡	`ScatterChartPro with big data amount`	165.9 ms	151 ms	+9.86%

alexfauquette · 2024-12-18T10:34:50Z

packages/x-charts/src/ChartContainer/useChartContainerProps.ts

@@ -45,20 +53,24 @@ export const useChartContainerProps = (
    ...other,
  };

-  const chartDataProviderProps: ChartDataProviderProps = {
+  const chartDataProviderProps: Omit<
+    ChartDataProviderProps<[UseChartCartesianAxisSignature<TSeries>], TSeries>,


For typing reasons, I set the cartesian plugin here, such that props related to cartesian axes are correctly typed

alexfauquette · 2024-12-18T10:35:38Z

packages/x-charts/src/ChartsAxis/ChartsAxis.tsx

+  const { xAxis, xAxisIds } = useXAxes();
+  const { yAxis, yAxisIds } = useYAxes();


You might see lots of them. The initial idea was to split x/y axes such that updating one does not impact the other. Finally I'm not sure it's that usefull

alexfauquette · 2024-12-18T10:37:45Z

packages/x-charts/src/ChartsSurface/ChartsSurface.test.tsx

+      <ChartProvider pluginParams={{ width: 100, height: 100, series: [] }}>
+        <ChartsSurface
+          ref={ref}
+          disableAxisListener // TODO: remove during v8 when charts store is always available


The store is available but not yet the axes, so removing it still make the CI crash

alexfauquette · 2024-12-18T11:30:57Z

packages/x-charts/src/context/PolarProvider/PolarProvider.tsx

+// 'use client';
+// import * as React from 'react';
+// import { computeAxisValue } from '../../internals/plugins/featurePlugins/useChartCartesianAxis/computeAxisValue';


I just commented the file about polar coordinate. They got introduced when starting the effort on radar chart.

They should be translated into a useChartPolarAxisPlugin

alexfauquette · 2024-12-18T11:32:53Z

packages/x-charts/src/internals/store/useStore.ts

-  if (skipError) {
-    // TODO: Remove once store is used by all charts.
-    // This line is only for `useAxisEvents` which is in the surface of the Gauge.
-    // But the Gauge don't have store yet because it does not need the interaction provider.
-    // Will be fixed when every thing move to the store since every component will have access to it.
-    // @ts-ignore
-    return context?.store;
-  }


That one is good to remove 🎉

alexfauquette added 18 commits November 26, 2024 16:54

WIP

a162ca4

extract params

7948389

continue

5bd1ebe

Merge remote-tracking branch 'upstream/master' into continue-pluggins

963c717

fix

83f736c

fix resize

f4cece8

fixes

85946e7

prettier

9e466b6

WORK

867ed2a

WIP

b6e2206

scripts

2c345c0

WIP

efdc030

Merge remote-tracking branch 'upstream/master' into all-pluggin-way

1410493

scripts

f6ca0c0

fix-ts

4421932

scripts

14e44e2

Try zoom feature

e2f138f

Merge remote-tracking branch 'upstream/master' into all-pluggin-way

5f8b332

alexfauquette added 2 commits December 12, 2024 18:19

all empty axes

1d17108

remove-proptypes

d17f173

alexfauquette force-pushed the all-pluggin-way branch from afd23e6 to d17f173 Compare December 12, 2024 17:28

JCQuintas reviewed Dec 16, 2024

View reviewed changes

alexfauquette added 4 commits December 16, 2024 16:31

Add the zoom panning

1050ec6

bring back series colors

6dc78ba

propagate series config

2f088e6

fix ts

c2261cc

alexfauquette marked this pull request as ready for review December 17, 2024 12:09

Merge remote-tracking branch 'upstream/master' into all-pluggin-way

c55fcd9

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 17, 2024

alexfauquette added 2 commits December 17, 2024 13:18

run script

956f63f

Remove plugins from proptypes to avoid wrong type resolution

15ef21c

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 17, 2024

alexfauquette added 3 commits December 17, 2024 15:35

fic rsc

c34d9e4

Avoid duplicated series config

992ab59

fix tests

9b15a35

zannager added the component: charts This is the name of the generic UI component, not the React module! label Dec 17, 2024

alexfauquette added 3 commits December 17, 2024 23:54

fix ts

4d46505

fix-import

b4ce8a1

Merge remote-tracking branch 'upstream/master' into all-pluggin-way

12fadbe

alexfauquette added 3 commits December 18, 2024 11:44

bring back chart container proprtypes

b8b7942

remove unused props type from gauge

132bd85

remove seriesConfig

1450039

alexfauquette commented Dec 18, 2024

View reviewed changes

Cleanup zoom API

b38dc27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Experiment with plugins for series+axes #15865

[charts] Experiment with plugins for series+axes #15865

alexfauquette commented Dec 12, 2024 •

edited

Loading

mui-bot commented Dec 12, 2024 •

edited

Loading

JCQuintas Dec 16, 2024

github-actions bot commented Dec 17, 2024

codspeed-hq bot commented Dec 17, 2024 •

edited

Loading

alexfauquette Dec 18, 2024

alexfauquette Dec 18, 2024

alexfauquette Dec 18, 2024

alexfauquette Dec 18, 2024

alexfauquette Dec 18, 2024

		const { xAxis, xAxisIds } = useXAxes();
		const { yAxis, yAxisIds } = useYAxes();

[charts] Experiment with plugins for series+axes #15865

Are you sure you want to change the base?

[charts] Experiment with plugins for series+axes #15865

Conversation

alexfauquette commented Dec 12, 2024 • edited Loading

Modifications

Renaming that could be extracted

Logic

Instance

Renaming plugins

Selector usage

Zoom API

mui-bot commented Dec 12, 2024 • edited Loading

JCQuintas Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024

codspeed-hq bot commented Dec 17, 2024 • edited Loading

CodSpeed Performance Report

Merging #15865 will improve performances by 9.86%

Summary

Benchmarks breakdown

alexfauquette Dec 18, 2024

Choose a reason for hiding this comment

alexfauquette Dec 18, 2024

Choose a reason for hiding this comment

alexfauquette Dec 18, 2024

Choose a reason for hiding this comment

alexfauquette Dec 18, 2024

Choose a reason for hiding this comment

alexfauquette Dec 18, 2024

Choose a reason for hiding this comment

alexfauquette commented Dec 12, 2024 •

edited

Loading

mui-bot commented Dec 12, 2024 •

edited

Loading

codspeed-hq bot commented Dec 17, 2024 •

edited

Loading