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

✨ (grapher) prevent scrolling while showing tooltips / TAS-528 #3678

Closed
wants to merge 1 commit into from

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Jun 3, 2024

Line charts and stacked area charts listen to move events on mobile to show a tooltip, but don't prevent scrolling, which makes the experience of interacting with the tooltip a bit jumpy (see video below).

This PR makes it so that scrolling is disabled when interacting with the chart area.

Implementation detail: All React event listeners are by default passive, which means that calling event.preventDefault() has no effect. That's why we need to attach the event listener to the DOM ourselves (technically, only the move event needs this but I moved them all to the componentDidMount method so that they're all defined in the same place)

line-tooltips.mov

Copy link
Member Author

sophiamersmann commented Jun 3, 2024

@owidbot
Copy link
Contributor

owidbot commented Jun 3, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-mobile-tooltips-prevent-scrolling

SVG tester:

Number of differences (default views): 1342 (9e2931) ❌
Number of differences (all views): 685 (1f9cfe) ❌

Edited: 2024-06-24 09:05:23 UTC
Execution time: 1.26 seconds

@sophiamersmann sophiamersmann force-pushed the mobile-tooltips-prevent-scrolling branch from 61b4d11 to 8234922 Compare June 3, 2024 14:33
@sophiamersmann sophiamersmann marked this pull request as ready for review June 5, 2024 12:50
@sophiamersmann sophiamersmann changed the title ✨ (grapher) prevent scrolling while showing tooltips ✨ (grapher) prevent scrolling while showing tooltips / TAS-528 Jun 6, 2024
Copy link

@sophiamersmann sophiamersmann force-pushed the mobile-tooltips-prevent-scrolling branch from 8234922 to 05259ee Compare June 6, 2024 12:51
Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Ah interesting, I didn't know that about react event handlers. This looks good and seems to work as advertised but I lack the expertise to know if there are any potential issues with this. I think you can safely merge this but if you want a review from someone who understands this stuff better then ping Martin :)

Copy link
Contributor

@rakyi rakyi left a comment

Choose a reason for hiding this comment

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

I haven't done this exact thing before myself, so I'm not sure if there are any gotchas hiding here. However, I know there is a great library for floating elements I used before that has a component for this.

Is there a chance that whatever we are already using for tooltips has something built-in as well? Otherwise, there is also react-remove-scroll in case we are not happy with what we did now, or we need a more reusable/battle tested solution.

@sophiamersmann sophiamersmann force-pushed the mobile-tooltips-prevent-scrolling branch from 6e42cb1 to edbc87d Compare June 24, 2024 08:44
@sophiamersmann
Copy link
Member Author

Thanks for your reviews! This is good to go but I want to ship it in concert with follow-up PRs, so closing it for now, but will be continued...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants