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

Prevent vertical scrolling while swiping on Safari iOS #129

Closed
wants to merge 6 commits into from
Closed

Prevent vertical scrolling while swiping on Safari iOS #129

wants to merge 6 commits into from

Conversation

JiiB
Copy link

@JiiB JiiB commented Mar 13, 2019

I'm creating this PR because I have the same problem as described here: #127
On iOS Safari it was possible to scroll vertically while swiping horizontally.

It's now possible to disable the onSwiping function if the initial swipe direction is up or down. The onSwiping function will only be called if the initial swipe direction is left or right.

Usage

<Swipeable
  onSwiping={swipingHandler}
  preventScrollOnHorizontalSwipe 
/>

Tasks:

  • Added new prop and functionality
  • Updated the docs
  • Added new tests
  • Added an example
  • Updated functionality for useSwipeable (not sure if this is even possible with my current implementation)

Notes:

  • These changes are fully backward compatible
  • Prettier automatically formatted the README.md. I personally like it better, but I could undo it.
  • I could not get it to work for the Hooks API useSwipeable


## Development

Initial set up, with `node 10+`, run `npm install`.

Make changes/updates to the `src/index.js` file.

***Please add tests if PR adds/changes functionality.***
**_Please add tests if PR adds/changes functionality._**
Copy link
Author

Choose a reason for hiding this comment

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

Ok prettier changed it to italic automatically. I can undo this of course

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think i may need to exclude readme from prettier, lol

@hartzis
Copy link
Collaborator

hartzis commented Mar 13, 2019

@JiiB this is a great start! i too am working on this and your PR has pointed out some interesting things.

What I've discovered so far:

Working from your approach I'm exploring if there is a way to do the v4 event listener approach.

  • so far unsuccessful attempting to attach the touchmove listener to the e.target from the touchstart event.

@hartzis
Copy link
Collaborator

hartzis commented Mar 13, 2019

updated comment in #127 (comment)

@hartzis
Copy link
Collaborator

hartzis commented Mar 14, 2019

Hey @JiiB thank you for help in identifying and working on this!

Based on your solution and a lot research this afternoon, like ~4hrs searching the web, I created #130.

Please let me know what you think about the these two different approaches.

Current thoughts:

  • try to avoid adding another prop if it is not "truly" necessary
  • since the v4 approach works, by random happenstance, i think mimicking that behavior would be the best approach since it is "battle tested" already
  • setup single touchmove event listener #130 works for useSwipeable hook

@JiiB
Copy link
Author

JiiB commented Mar 15, 2019

@hartzis Thanks for your response. I was very busy yesterday.

I see your point, that it does not make that much sense to add an additional prop just for this special use case. Unfortunately your PR #130 does not resolve my issue in safari 🤔 It is still possible to scroll vertically while swiping horizontally.

I'm going to investigate again and keep up updated.

@hartzis
Copy link
Collaborator

hartzis commented Mar 15, 2019

@JiiB Thanks for the update, sorry to hear that #130 didn't resolve the issue, how are you testing these solutions?

#130 does work for me locally when testing using the xcode simulator. I was able to prevent vertical scrolling while swiping horizontally.

@hartzis
Copy link
Collaborator

hartzis commented Mar 18, 2019

@JiiB closed #130 in favor of #131

@JiiB
Copy link
Author

JiiB commented Mar 18, 2019

@hartzis I'm going to close this PR since your changes in #131 fixed the problem with iOS safari 👍

@JiiB JiiB closed this Mar 18, 2019
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.

2 participants