-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Control the item tooltip #20617
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-20617--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #20617 will not alter performanceComparing Summary
Footnotes |
|
The tooltip doesn't seem to anchor to its item on scroll: Screen.Recording.2025-12-16.at.11.25.00.mov |
|
|
||
| :::warning | ||
| Make sure the tooltip `trigger` is set to `"item"`. | ||
| Otherwise you might control the item tooltip and render an axis tooltip |
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.
What do you mean by this? If I set it to axis it doesn't seem to work:
Screen.Recording.2025-12-16.at.11.38.37.mov
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.
That's exactly what I meant: it does not work.
Internally the tooltip item is correctly controlled. But you render an axis tooltip so nothing happend
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.
That's exactly what I meant: it does not work.
I see, but I think this wording is a bit confusing, then:
Otherwise you might control the item tooltip and render an axis tooltip
IMO, we should say something like: "Make sure the tooltip trigger is set to "item", otherwise no tooltip will be shown".
I propose an hybrid approach between a
@bernardobelchior If you want I can extract this commit in a dedicated PR. It's just that with controlled tooltip it's much easier to reproduce the issue due to scrolling |
| anchorRef.current = document.createElementNS('http://www.w3.org/2000/svg', 'rect'); | ||
| svgRef.current.appendChild(anchorRef.current); |
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 create this using a portal? I'm worried this will cause issues because we're never removing this rect.
Also, it seems we're creating this rect even if it isn't needed:
Screen.Recording.2025-12-16.at.17.20.44.mov
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.
Why use a portal?
Yeah, I think that's a good solution 👍
Maybe it wouldn't be a bad idea because I'm seeing some weird behavior and maybe it's easier to iterate on a smaller PR: Not sure why, but this doesn't seem to be properly positioned in mobile: Screen.Recording.2025-12-16.at.17.18.08.movAlso, the synchronized demo doesn't work very well in mobile because the tooltip item disappears on touch end. I wonder if we should try to keep showing it after touch end to ensure touch users can see it: Screen.Recording.2025-12-16.at.17.18.49.mov |
That' because we add extra space on mobile to avoid having the tooltip hidden under the user thumb
I don't see how to do that. When user don't touch the chart it should disapear. We could add extra parameter or demos to either add a pending time where the tooltip is still visible, or a tooltip that wait for a click outside to close. |
Makes sense 👍
Yeah, I thought we could do something with the demo just so it's visible; but then it would be weird because it wouldn't show the correct behavior |
| ); | ||
| }} | ||
| /> | ||
| <pre>Tooltip Item: {JSON.stringify(barTooltipItem, null, 2)}</pre> |
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.
| <pre>Tooltip Item: {JSON.stringify(barTooltipItem, null, 2)}</pre> |
| ); | ||
| }} | ||
| /> | ||
| <pre>Tooltip Item: {JSON.stringify(pieTooltipItem, null, 2)}</pre> |
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.
| <pre>Tooltip Item: {JSON.stringify(pieTooltipItem, null, 2)}</pre> |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Fix #20187
Changelog