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 the line item highligh into a dedicated component #10117

Merged
merged 14 commits into from
Aug 29, 2023

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Aug 22, 2023

Linked to #9228 (comment)

Since the mark element is CPU consuming (a lot of tinny items to render)

If we only want to display the highlighted ones, it's better to render only those instead of rendering them all and hiding the other with CSS.

I let it for review without updating all the demos, such that it stays as small as possible.

I'm also uncertain about naming: showMark, disableHighlight, and disableLineItemHighlight.

Maybe it would be better to replace the series property renderHighlight with disableHighlight.

Todo

  • Define a way to choose between this new approach and the mark elements
  • Document it
  • Add this to sparkline
  • Update all demo

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

mui-bot commented Aug 22, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10117--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -192.7 -12.9 -42.7 -81.32 61.634
Sort 100k rows ms 674 1,487 1,416.1 1,232.16 293.446
Select 100k rows ms 698 880.4 770.3 766.92 64.644
Deselect 100k rows ms 143 246.2 203 196.76 33.505

Generated by 🚫 dangerJS against a18f51c

@alexfauquette alexfauquette marked this pull request as ready for review August 24, 2023 10:33
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Very nice improvement! 👍

Leaving a few syntax suggestions as well as a suggestion for the prop name inversion. 😉

docs/data/charts/lines/lines.md Outdated Show resolved Hide resolved
docs/data/charts/lines/lines.md Outdated Show resolved Hide resolved
packages/x-charts/src/LineChart/LineChart.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/LineChart/LineHighlightPlot.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/models/seriesType/line.ts Outdated Show resolved Hide resolved
packages/x-charts/src/models/seriesType/line.ts Outdated Show resolved Hide resolved
{ "name": "LineHighlightElement", "kind": "Function" },
{ "name": "lineHighlightElementClasses", "kind": "Variable" },
{ "name": "LineHighlightElementClasses", "kind": "Interface" },
{ "name": "LineHighlightElementOwnerState", "kind": "Interface" },
Copy link
Member

Choose a reason for hiding this comment

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

Is there a point in exporting the OwnerState type in charts codebase? 🤔
I don't see it exported on pickers or data grid. 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, I probably added export by reflex when defining those interfaces

@alexfauquette alexfauquette merged commit ced19b2 into mui:master Aug 29, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants