search

FormidableLabs / react-swipeable

React swipe event handler hook
https://commerce.nearform.com/open-source/react-swipeable
MIT License
1.99k stars 146 forks source link

Preventing default event only in certain cases #224

Closed Hrach-H closed 2 years ago

Hrach-H commented 3 years ago

So, I have an issue with the latest version (6.*) that prevents me from migrating and I was wondering if anyone could help me.

I have a navigation bar that has a horizontal scroll that can also be swiped up/down. In the "onSwiping" handler I checked if it was the "first", the direction was either "Up" or "Down" and the event was cancelable in which case I called "preventDefault" on it. Now I know that it sounds "hacky" but it was the only way I could make it work.

Is it possible to achieve the same in the new version or am I stuck with the old one?

hartzis commented 3 years ago

@Hrach-H Thanks for posting the issue!

Curious if the new bug fix helped you or what else did you figure out to close the issue?

Hrach-H commented 3 years ago

@hartzis ,

I haven't yet tried it out, but I think I can just set 'onSwipedUp' and 'onSwipedDown' and set 'preventDefaultTouchmoveEvent' to 'true' and it should work.

However I have another similar issue that is currently solvable only with 'touch-action' property. Thing is I have to support iOS versions < 13. I wish we had more flexibility with cancelling the events, like in the previous versions.

hartzis commented 3 years ago

I wish we had more flexibility with cancelling the events, like in the previous versions.

I'd love to explore this a bit and see if we can update react-swipeable to make this easier again.

What is swipeable missing that it previously had?

However I have another similar issue that is currently solvable only with 'touch-action' property.

Glad to hear touch-action helps out here, it is an often overlooked aspect. And we only recently added it to the readme

Hrach-H commented 3 years ago

What is swipeable missing that it previously had?

Imagine we have a list of horizontally swipeable containers that when swiped left/right exposes additional functionality like delete, "like", etc. Because they're in a list, the page has a vertical scroll. So as I've stated previously, what you would do now, with the updated version of react-swipeable, is just have an "onSwiping" handler that sets the x axis for the transform and then run the callbacks in the "onSwiped" handler (at least that's what I came up with). This is also true for the previous version, however in order to prevent the default scroll when the container is already being swiped you would have to set the touch-action: pan-y, which is not supported on apple devices that run iOS < 13. While previously I would just prevent the default event while the container was being swiped. In the new version, if I set the "preventDefaultTouchmoveEvent" to true it would block the regular (vertical) scroll, which is not what I want.

hartzis commented 3 years ago

@Hrach-H Thank you for the great explanation and detail.

I can see how preventDefaultTouchmoveEvent in this scenario is too all encompassing.

💭 maybe a directional approach for preventDefault?

💭 was also curious if manually calling event.preventDefault when you need it could work?

Hrach-H commented 3 years ago

@hartzis ,

Sorry for responding so late.

I like the directional approach a lot! Regarding the swipe event - I've tried that, but because the event is passive, preventDefault cannot be called on it.

hartzis commented 3 years ago

Regarding the swipe event - I've tried that, but because the event is passive, preventDefault cannot be called on it.

We currently "hard code" passive based off preventDefaultTouchmoveEvent. https://github.com/FormidableLabs/react-swipeable/blob/f4d19af0059cee5f59fc049a3b6e6b8696fbd831/src/index.ts#L253 https://github.com/FormidableLabs/react-swipeable/blob/f4d19af0059cee5f59fc049a3b6e6b8696fbd831/src/index.ts#L228

I remember thinking about the "hard coded" nature of this during the implementation.

eventOptions

🤔 💭 We could possibly introduce a new prop to control the addEventListener options, ie eventOptions.

Hrach-H commented 3 years ago

Yes, this would also work. Both the solutions you offered would solve my issue, and would allow me to migrate to the newer version. :partying_face:

hartzis commented 2 years ago

Please check out v7.0.0 and the new props to see if they can help out now.