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

[fix] https://github.com/kenwheeler/slick/issues/1252 #2521

Merged
merged 2 commits into from
Apr 19, 2017

Conversation

marcelombc
Copy link
Contributor

Description

This PR fixes the problem of carousel sliding while drag-scrolling vertically on a touch device.
When the user scroll down and then swipe to the left or the right, the carousel swipes to left and right while scrolling which is quite annoying.
It's possible to see a video of the problem here and you can find the github issue here

Demo

Without the fix
With the fix

Note

You need to test these demos on touch devices

Credits

All the credits goes to @levymetal. Thank you for the code to fix this issue.

@simeydotme
Copy link
Collaborator

I haven't tried this, sorry I'm not able to do a full review... however, just looking at the commited changes it seems logical... but throwing a question out there:

Would this kill vertical carousels ?

@marcelombc
Copy link
Contributor Author

That's a good point @simeydotme. I had a small issue with that but is already fixed and the vertical and horizontal carousel is working has expected. Sorry for not test that scenario.

@semenets
Copy link

semenets commented Jan 3, 2017

I had the same problem today. When this PR will be ready for merge? :)

@bbodien
Copy link

bbodien commented Feb 3, 2017

Would also ❤️ to see this merged.

@dangelion
Copy link

@kenwheeler please merge this. Thanks

@kenwheeler
Copy link
Owner

@leggomuhgreggo can you verify this is all good?

@leggomuhgreggo
Copy link
Collaborator

leggomuhgreggo commented Apr 3, 2017

@kenwheeler happy to -- I'll holler at it this evening.

@leggomuhgreggo
Copy link
Collaborator

Apologies for the long delay. This passes all my testing, nice work @marcelombc

In answer to @simeydotme's vertical mode question, I checked and vertical mode doesn't currently support touch swipe (at least on my phone), and, in any case, vertical mode has only ever been partially supported; so we're good here I'd say.

Merging. Thanks everyone!

@leggomuhgreggo leggomuhgreggo merged commit cab526e into kenwheeler:master Apr 19, 2017
@FullstackJack
Copy link

Both examples are janky on Android. It seems that the fixed version prevents side to side scroll as well if you swipe too frequently.

@leggomuhgreggo
Copy link
Collaborator

@FullstackJack can you elaborate on what you're seeing? I think a slide transition has to finish, before a new one will register; so, I wonder if that's what's goin on.

@yanwsh
Copy link

yanwsh commented Jan 31, 2018

This fix make swap very difficult on mobile device, you have to make exact horizontal swap, otherwise it won't work, this is poor user experience. Hope it can provide an option let user setup vertical swap length compare value. Right now it's fixed number which is 4. I think it's better to be like 15.

 if (!_.options.verticalSwiping && !_.swiping && verticalSwipeLength > 4) {
   _.scrolling = true;
   return false;
 }

@rssteffey
Copy link

I have to second what @yanwsh said above. This fix has made scrolling a fullscreen div incredibly frustrating. (On an iPhone X, at least). Are there any plans to add a tolerance threshold to this limitation?

@gstoyanov
Copy link

I can confirm the problem @yanwsh mentioned above. Found an open issue about it as well: #3464

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.