JedWatson / react-tappable

Tappable component for React
http://jedwatson.github.io/react-tappable/
MIT License
862 stars 90 forks source link

don’t reactivate the Tappable when moving back within the moveThreshold #85

Closed mattcolman closed 6 years ago

mattcolman commented 8 years ago

Hi, I think when you move outside the moveThreshold the Tappable should stay inactive for the remainder of the move. Returning back to within the moveThreshold should not re-activate the Tappable. Do we agree this is the expected functionality? I'm referencing iOS behaviour. Touch an app and quickly move to the left or right triggering a swipe. Moving the app back to its original source and releasing your finger will NOT open the app.

JedWatson commented 8 years ago

We actually referenced iOS when building this, I believe you're talking about the home screen which has unique behaviour (with regards to swiping)

If you do the same thing with a button (e.g. the < Back navigation in a header) - tap, move your finger away, and move it back again - it will re-activate the button, which is the same behaviour as react-select.

I guess that the behaviour is not desirable if the button is within a swipeable view, if we made that possible I think maybe the best approach would be to add a prop that lets you disable the reactivation rather than removing it in all situations.

mattcolman commented 8 years ago

Hey @JedWatson, thanks for the reply. Yep, makes sense. Seems like there are 2 different use cases so probably a single prop to switch between them would be good. If the Tappable is in a swipeable view, as you say, it should not reactivate and it should also change the default moveThreshold to 5 pixels (I think this is the iOS standard for this case). I could submit a PR if you agree with this logic?

mattcolman commented 7 years ago

@JedWatson I've had a deep dive into the code and arrived at a solution that works for me. As you suggested I've added a flag to disable reactivation. I've also added moveXThreshold and moveYThreshold which will take precedence over moveThreshold if defined.

In my own project I've created a component called TappableListView which just sets wraps Tappable with defaultProps of moveYThreshold: 5 and allowReactivation: false.

I've also added hooks for onDeactivate and onReactivate. These can be used if 'Tappable-active' is not enough (i.e. you want to style more than the Tappable itself.)

In saying all of that I noticed there is code detectScroll which probably should actually do all of this automatically. It appears that detectScroll will only work if the parent is a div with a fixed height and overflow set to scroll. In my case I have a list of Tappables overflowing off the page and detectScroll() is always false.

I do have another branch where I alter this method to work for my case, but I actually prefer not to use 'scroll detection' at all. It seems like a point of failure and in many cases an unnecessary overhead if your Tappable will never scroll.

Merry Christmas!

mattcolman commented 7 years ago

@dcousens fixed

dcousens commented 7 years ago

LGTM, will eventually merge unless NACK received

mattcolman commented 6 years ago

are there still plans to merge this?

dcousens commented 6 years ago

Could be a while before being published, unless someone else can test. The code looks fine though :+1: