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] Add support mobile tooltip #13043

Merged

Conversation

wzdorowa
Copy link
Contributor

@wzdorowa wzdorowa commented May 7, 2024

Resolves: #13041

Added tracking of touch events for correct operation on mobile devices. This works in case of an axis trigger.

This solution does not work for an item trigger. It is necessary to come up with a solution for this case.

Preview: https://deploy-preview-13043--material-ui-x.netlify.app/x/react-charts/tooltip/

@mui-bot
Copy link

mui-bot commented May 7, 2024

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

Generated by 🚫 dangerJS against ca26170

@JCQuintas JCQuintas added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels May 7, 2024
@JCQuintas
Copy link
Member

That's kinda cool, thanks for your contribution. :D

Do you have any insights on why item is not working? Did you try to make it work?

@wzdorowa
Copy link
Contributor Author

wzdorowa commented May 7, 2024

item doesn’t work because we don’t have an equivalent of mouseenter/leave events for touch.

The workarounds found don’t look very good and can lead to performance issues. So I didn’t add them.
Failed to add handlers to this file: https://github.com/mui/mui-x/blob/master/packages/x-charts/src/hooks/useInteractionItemProps.ts

@JCQuintas
Copy link
Member

@alexfauquette current issue with mouse events in item tooltip is that we trigger the event for every item, which prevents allowing the touch event to move freely and updating the tooltip based on which element is currently on focus. We can probably move the useInteractionItemProps to be more similar to the useAxisEvent instead if we want this behaviour to be present in the item tooltip as well.

@alexfauquette alexfauquette changed the title [charts][ChartsTooltip] Add support mobile tooltip [charts] Add support mobile tooltip May 13, 2024
@alexfauquette
Copy link
Member

We can probably move the useInteractionItemProps to be more similar to the useAxisEvent

I'm not sure to understand what you're proposing.

The useAxisEvent is a hook use at the level of the SVG because with axis scale we can easily go from mouse position to value and the opposite.

But for items, we don't have a simple way to map (x, y) to the item. That's why we rely on the SVG elements to catch the enter/leave/click

You are proposing to put a onMouseMove/onTouchMove and rely on the event.target to catch which element is getting the highlight? a solution with something like <path data-series-id="..." data-item-id="..." ... /> to transmit the information up to the root?

By the way, items are already visible if you click on them

I assume we can add this fix which is already an improvement, and fix the item aspect in another one

@JCQuintas
Copy link
Member

<path data-series-id="..." data-item-id="..." ... />

yes, I was thinking something like that.

I'm aware the items are visible on click, but this MR is about making "dragging" your finger around work seamlessly, without having to click again.

I assume we can add this fix which is already an improvement, and fix the item aspect in another one

Agree

@JCQuintas JCQuintas merged commit 7952980 into mui:master May 13, 2024
18 of 19 checks passed
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work 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.

[charts][ChartsTooltip] Tooltip not working on mobile
4 participants