Skip to content
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

[charts] Move interaction state in store #15426

Merged
merged 10 commits into from
Nov 18, 2024

Conversation

alexfauquette
Copy link
Member

Extracted from #15154 focusing only on the introduction of the store.

The PR can be read commit by commit.

The performances gain for the tooltip are not interesting in this PR, because I tried to keep it without breaking change. And the props of the axis tooltip are too generous. They provide the state for x and y axis. So as soon a the mouse move I need to update the state either for x or y. The performances of the tooltip will be done in the breaking change of #15154

The axis highlight get some performances improvement by having a component for the x-axis and another for the y-axis. Such that they re-render only if the correct value is updated.

@JCQuintas I noticed your concern about the naming of InteractionProvider. The plan is to go one provider per PR, and move all the typing and the logic into the pluggin folder. At the end it will be named ChartsDataProvider or whatever you name the component in your PR to split the ChartContainer

@alexfauquette alexfauquette added component: charts This is the name of the generic UI component, not the React module! v8.x labels Nov 14, 2024
@mui-bot
Copy link

mui-bot commented Nov 14, 2024

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

Generated by 🚫 dangerJS against 3fa4900

Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #15426 will degrade performances by 10.77%

Comparing alexfauquette:extracted-selector (3fa4900) with master (e151681)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 4 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master alexfauquette:extracted-selector Change
LineChart with big data amount 854.9 ms 958.1 ms -10.77%
ScatterChart with big data amount 594.4 ms 480.1 ms +23.82%

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice extraction, everything seems to work nicely.

I've left some comments for possible improvements.

I would also argue that everything new created inside the internals/ folder could either be in a root store/ folder or inside internals/store/.

Since the internals is quite bloated already without specific access patterns 😓

alexfauquette and others added 2 commits November 15, 2024 15:49
@alexfauquette
Copy link
Member Author

About renaming files, I would prefer to do that in a dedicated PR after merging this one and #15154

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2024
Copy link

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2024
@alexfauquette alexfauquette merged commit 0da596e into mui:master Nov 18, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! v8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants