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

feat: fire the onTouchStart prop #464

Merged
merged 1 commit into from
May 21, 2019
Merged

Conversation

sangle7
Copy link
Contributor

@sangle7 sangle7 commented Jan 30, 2019

Platforms affected

all

What does this PR do?

fire the onTouchStart prop in carousel

the onTouchStart prop didn't fire in the previous code, which may end in unwanted result.

an online demo here: https://snack.expo.io/@sanglewang/react-native-snap-carousel-demo

in the case, we have two nested vertical scrollview, on android only the outside scroll view can be scrolled. we add a state to control the scrollEnabled prop to hack it. since the onTouchStart prop did not fire in the carousel, the state will never turn to false, and the carousel cannot mannully scroll.

I was wondering why the other event props (onScrollEndDrag, onMomentumScrollEnd etc. ) are fired in the component and the onTouchStart is not. Is there any protential problem i ignored ?

What testing has been done on this change?

Tested features checklist

@bd-arc
Copy link
Contributor

bd-arc commented Feb 15, 2019

Hi @sangle7,

Thank you for the PR!

Well, there shouldn't be any issue in theory, but I remember seeing weird things with the autoplay feature...

Have you made sure that the autoplay was still working properly on iOS and Android when a onTouchStart callback was defined?

@sangle7
Copy link
Contributor Author

sangle7 commented Feb 18, 2019

yes, i've tested on both Android and iOS device. Autoplay works well.😃

@bd-arc bd-arc merged commit a04bd7e into meliorence:master May 21, 2019
@bd-arc
Copy link
Contributor

bd-arc commented May 21, 2019

Shipped in version 3.8.0.

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