-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Add bar batch renderer #20457
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
base: master
Are you sure you want to change the base?
Conversation
|
Deploy preview: https://deploy-preview-20457--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #20457 will not alter performanceComparing Summary
Footnotes |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1d51405 to
a152903
Compare
d6e2d12 to
e6c01d8
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
856ae32 to
f8701da
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
f8701da to
ae86542
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
ae86542 to
185ca1f
Compare
67f4390 to
85e75cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely, gj on the tooltip/highlight as well 👍
I've left some comments on possible improvements
| import { getBarItemAtPosition } from './getBarItemAtPosition'; | ||
| import { useStore } from '../internals/store/useStore'; | ||
|
|
||
| export function useInteractionItemProps() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use this and useOnItemClick on the default bar renderer as well?
It feels like it could be useful if we render stuff on top of charts, or if using things like canvas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like it could be useful if we render stuff on top of charts
Well, if we want to render stuff on top of charts and don't want them to influence the clicks, we should add pointer-events: none to that stuff. I think it would be weird to hijack clicks from elements that would normally receive them.
It's unfortunate we need to maintain two code paths, but I suppose that's the price to pay for better performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the performance much better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have another option. If we want to implement a batch bar plot like the one in this PR, we can't use onClick event to detect clicks, so the only way is to calculate the clicked bar like we do in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I don't mean that.
I meant to say that we should maybe use the way it is calculated in this PR (based on pointer only) everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but it would be a breaking change because the event source would have changed.
It also changes event handling slightly, which might break stuff that users are relying on.
I guess this is something that we'd need to investigate deeper
cd32216 to
8cd7e48
Compare

Part of #12960.
Here's a PR where bar charts default to the batch renderer so you can test it and check Argos.
Differences from the individual bar plot:
Before/After
Before
After