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

fix(zoom): prevent wheel interfering with pinch #1639

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

kiliancs
Copy link
Contributor

🐛 Bug Fix

  • Prevent wheel interfering with pinch

    Using a macbook, a touchpad pinch gesture will trigger useGesture's pinch event, as desired, but it will also trigger the wheel event. Except in Safari.

    With this change, the wheel event handler bails if a pinch gesture is ongoing.

    Tested in Firefox, Chrome and Safari on a macbook.

    Fixes: Zoom pinch spasming  #1638

@kiliancs kiliancs mentioned this pull request Jan 25, 2023
@kiliancs kiliancs changed the title feat(zoom): prevent wheel interfering with pinch fix(zoom): prevent wheel interfering with pinch Feb 1, 2023
Using a macbook, a touchpad pinch gesture will trigger `useGesture`'s
pinch event, as desired, but it will also trigger the wheel event.
Except in Safari.

With this change, the wheel event handler bails if a pinch gesture is
ongoing.

Fixes: airbnb#1638
Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Hey @kiliancs thanks so much for the bug report and digging in to fix this nuance 🙌 apologies for the review delay, have been absolutely slammed with work.

This lgtm but I think CI somehow didn't get triggered. Would it be possible for you to push another commit or rebase to trigger? if not I'm happy to do it since you already did the hard work 😅 thanks again

@williaster
Copy link
Collaborator

hey @kiliancs just wanted to ping you once more to see if you could rebase and push again to trigger CI. if not I'll do that tomorrow and land this. thanks again!

@kiliancs
Copy link
Contributor Author

Hi there! Sorry, I am currently on vacation and will probably not be able to get to it until next week, so if you don't mind going ahead with the rebase, that'd be great.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

looks like I can't push to your remote easily, so just going to make a UI suggestion and commit to nudge CI

packages/visx-zoom/src/Zoom.tsx Outdated Show resolved Hide resolved
@github-actions
Copy link

🎉 This PR is included in version v3.1.1 of the packages modified 🎉

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

Successfully merging this pull request may close these issues.

Zoom pinch spasming
2 participants