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

Implement lockScrollTimeout #260

Closed
wants to merge 5 commits into from
Closed

Implement lockScrollTimeout #260

wants to merge 5 commits into from

Conversation

mkuczera
Copy link

@mkuczera mkuczera commented Feb 3, 2018

Gives the implementation more control over the lock duration

Gives the implementation more control over the lock duration
@bd-arc
Copy link
Contributor

bd-arc commented Feb 4, 2018

Hi @mkuczera,

Thanks for the PR, but it shouldn't be needed. The 1 second timeout is a safety measure just in case something goes wrong (which happens sometimes on Android in debug mode).

If no particular issue arises, the lock will be released as soon as an item has been snapped to (see lines 743-755).

@mkuczera
Copy link
Author

mkuczera commented Feb 4, 2018

Hi @bd-arc ,
we had some issues regarding Android release versions with a horizontal react-native-snap-carousel including fullwidth views with more complex carousel items. On devices we experienced android issues with fast swiping, lagging and sometimes not snapping at all. Enabling the Snaplock resulted in the best experience, but the 1 Second timeout was far to much. So implementing this option for the dev shoudn´t bring any disadvantages, or?

@bd-arc
Copy link
Contributor

bd-arc commented Feb 6, 2018

Hey @mkuczera,

Be aware that if you implement a shorter timeout you might experience unwanted side effects (onSnapToItem() not triggered for example). The lock is supposed to be released as soon as the carousel finishes snapping to the next item, which shouldn't take 1 second.

Still, I definitely understand your need. One day (#203), we might not need all those hacks... For now, I'm ok to merge this but as an undocumented feature. I don't want users to use this prop without understanding what's at stake; they would end up opening issue after issue.

Can you push another PR that includes only your changes to Carousel.js? This will keep the commit history clean ;-)

@mkuczera
Copy link
Author

mkuczera commented Feb 8, 2018

Hi, thanks for your feedback. I removed the Docs Hint for this Prop.

@bd-arc
Copy link
Contributor

bd-arc commented Feb 8, 2018

@mkuczera Thanks but, as I said, I would prefer if you could create a new fork and a new PR with only one commit ;-) (I want to keep the commit history clean and to avoid the revert ones)

@bd-arc
Copy link
Contributor

bd-arc commented Apr 12, 2018

@mkuczera Prop lockScrollTimeoutDuration has been implemented in version 3.7.0.

@bd-arc bd-arc closed this Apr 12, 2018
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