-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
✨ (grapher) pin tooltips to the bottom on mobile / TAS-664 #4082
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @sophiamersmann and the rest of your teammates on Graphite |
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 ✅ Edited: 2024-11-14 17:01:06 UTC |
7221299
to
2f83f44
Compare
d68a5c5
to
bae4058
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.
Really nice work here, I like the new behavior.
I tested this on desktop and mobile. On desktop I didn't notice any issues. On mobile I noticed two:
- When clicking on "Learn more about this data" on a data page (which scrolls down), the tooltip stays visible even though the chart itself leaves the viewport. Maybe the IntersectionObserver doesn't fire for programmatic scrolls?
- The tooltip also stays visible when clicking the download button and the share button
I'm wondering if there is a better way to register click events - maybe use a {capture: true}
event handler to catch them all??
packages/@ourworldindata/grapher/src/tooltip/TooltipContents.tsx
Outdated
Show resolved
Hide resolved
Ah, damn it. I've thought about using a capture event but I don't think that works because interacting with the chart shouldn't dismiss the tooltip. If we wanted to use a capture event for the document, we would also need to use capture events for chart event handlers, which I thought was too much of a weird constraint. |
I think a capturing event handler would work? I agree about that clicking inside the chart area should not dismiss the tooltip, but that we can handle inside the event handler. When searching through the code just now, I saw that See SettingsMenu here - inside the handler, it checks whether the click target is inside the settings menu. We can do a similar thing for the charting area also. |
Ah, that's not something I considered. Yes, that should work! |
9a86490
to
1e82efa
Compare
I'm currently confused how dismissing the tooltip works for chart types other than line & stacked? How does it work for maps, for example - they don't seem to have a handler inside |
Line charts and stacked area charts are special because they implement touch events so you can hold your thumb down, and on movement, the tooltip gets updated. Other chart types don't do this. They only register mouse enter and leave events. So, if you press down on an element on mobile, then the mouse enter event is fired (showing the tooltip), and if you click anywhere else, then the mouse leave event is fired (dismissing the tooltip). |
014a681
to
f7a54a5
Compare
The code is now using a capture event handler, but for some reason it doesn't get fired when interacting with the timeline. I'm not sure why – that's why, when interacting with the timeline, the tooltip still gets dismissed explicitly. |
@marcelgerber this is good to go from my side. Do you think we can get this over the finish line? No worries if you would need too much to finish the review though :) |
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.
Yes, good to go now! 🚀
0822d26
to
9bd1fae
Compare
0804147
to
613742d
Compare
Pin tooltips to the bottom of the screen on mobile for a more pleasant UX. When pinned to the bottom, the tooltip hides less of the chart area and isn't "in the way" when interacting with a chart. This also fixes several overflow issues for very tall tooltips.
Summary
Notes
Caveats
Example charts
Testing
This behaviour only kicks in for touchscreen devices, so it's best to look at these changes on an actual phone.
Drive-by changes
Screenshot