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 item highligh feature to plugin system #16211

Merged
merged 21 commits into from
Jan 29, 2025

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jan 16, 2025

The PR is divided in commits to avoid having to go through all the definition updates.

There is nearly no breaking change in this PR, except for the hook to get directly access to the context, which got removed. (because the context does not exist anymore).

The only feature they could miss from this hook is the one in useHighlightStateGetter which returns { isHighlighted: (item) => boolean, isFaded: (item) => boolean}. which is more flexible than the useItemHighlighted returning { isHighlighted: boolean, isFaded: boolean}

Changelog

Breaking changes

The useHighlighted hook got removed.
This hook provided direct access to the internals of the highlight management. We now recommend using the control API, and the useItemHighlighted/useItemHighlightedGetter.
More details in the migration guide

If your use-case requires more advanced options, please open an issue.

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Jan 16, 2025
@mui-bot
Copy link

mui-bot commented Jan 16, 2025

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.

Small comments, feature seems to be working

@alexfauquette
Copy link
Member Author

@JCQuintas I've exported the lower level hook and added instruction in the migration guide, because some users rely on the removed hook to highlight some item in their tooltip

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.

small suggestion

alexfauquette and others added 2 commits January 17, 2025 11:58
Copy link

codspeed-hq bot commented Jan 17, 2025

CodSpeed Performance Report

Merging #16211 will degrade performances by 73.35%

Comparing alexfauquette:plugins-again (b1931e9) with master (cbe47de)

Summary

⚡ 5 improvements
❌ 1 (👁 1) regressions

Benchmarks breakdown

Benchmark BASE HEAD Change
BarChart with big data amount 923.9 ms 544.3 ms +69.74%
👁 BarChartPro with big data amount 218 ms 818.3 ms -73.35%
LineChart with big data amount 975.6 ms 317.2 ms ×3.1
LineChartPro with big data amount 528.6 ms 137.9 ms ×3.8
ScatterChart with big data amount 479.2 ms 392 ms +22.26%
ScatterChartPro with big data amount 125.7 ms 53 ms ×2.4

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 20, 2025
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 Jan 20, 2025
Copy link

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 20, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 20, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 27, 2025
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 Jan 28, 2025
@alexfauquette
Copy link
Member Author

The HEAD is not relevant because the script did not run on master since the update. But we can have an idea by looking at the HEAD results on #16374

The Line and Bar chart (pro and MIT) take 30ms more. I trie to compare the render of this branch and the master on my device, but I get the opposite. The new version takes less time to render

@alexfauquette alexfauquette merged commit a9c4eb1 into mui:master Jan 29, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants