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

[ touch idea ] disabling horizontal frame movement when scrolling vertically #119

Open
victorbyttebier opened this issue Jun 21, 2019 · 12 comments
Labels

Comments

@victorbyttebier
Copy link

victorbyttebier commented Jun 21, 2019

Hello!

If a user scrolls vertically on a touch device, it would be nice to disable the active carousel frame by only allowing horizontal scrolling if demanded. I think changing to the next frame if the horizontal touchevent ended, could be useful and leaves that unwanted behavior.

Kudos for the library

@victorbyttebier
Copy link
Author

Deleting line 133 stopped the unwanted behavior when scrolling vertically on a mobile device.
133 // this.moveFramesBy(deltaX, deltaY)

It would be nice to set this as a Carousel attribute.

@amio
Copy link
Owner

amio commented Jun 22, 2019

Use axis={'x'} attribute would disable vertical scrolling, could it meet your need?

https://j22o38y3lv.codesandbox.io

@victorbyttebier
Copy link
Author

My problem is that the carousel is too sensitive. If the user wants to scroll down not to look at carousel part, it also moves horizontally. By deleting the line it doesn't move the frames instantly. I'll add 2 gifs to explain visually.

@victorbyttebier
Copy link
Author

moving

w/o instant framemoving
nomoving

@amio
Copy link
Owner

amio commented Jun 22, 2019

Thanks @victorbyttebier!

I think it's a regression, will check on this later.

@amio amio added the bug label Jun 22, 2019
@lluisrojass
Copy link
Contributor

is this being investigated? mind if i give it a look?

@amio
Copy link
Owner

amio commented Aug 11, 2019

That would be nice! @lluisrojass

@besserwisser
Copy link

@illuisrojass have you found something?

@lluisrojass
Copy link
Contributor

lluisrojass commented Oct 3, 2019

@besserwisser @amio @victorbyttebier

The bug is caused because we are using PageY/PageX to calculate deltas.

The scenario is that a user begins scrolling down the page vertically, and because the page scrolls, our deltaY remains constant (+/- 10 px) as the page scrolls. It doesn't take much for the deltaX to overtake deltaY here and commence scrolling of the carousel (you can see this happen in the GIF submitted by @victorbyttebier above). At this point you start getting nasty errors in your console because we are attempting to cancel an un-cancellable event.

Screen Shot 2019-10-02 at 9 21 09 PM

We should be using clientX & clientY to calculate deltaX & deltaY since top left corner of the viewport is independent of future scrolling. Is there any reason to not do this? @amio

Even after making the necessary changes there is still a scenario where a user's touch movement triggers later carousel movement which makes for some jagged UI (when deltaX overtakes deltaY).

2019-10-02 21 37 27

other patterns of movement cause even more intrusive issues ☹️

We need some way to identify a touch should be ignored, i think the best way to do it is to calculate the angle on the first touchMove. we can do this easily via const angle = Math.abs(Math.atan(deltaY / deltaX));. If this angle is above a certain value we can ignore the whole touch swipe. Users could pass a prop to define how large the area is that could constitute a carousel swipe.

would a change like this be considered breaking?

@amio
Copy link
Owner

amio commented Oct 4, 2019

We should be using clientX & clientY to calculate deltaX & deltaY since top left corner of the viewport is independent of future scrolling.

👍 Great point, we should use clientX & clientY for a more reliable touch detection.

identify a touch should be ignored ... (by) calculate the angle on the first touchMove.

Usually it's a very small amount movement in first touchMove event. Not sure if we can guess user's intentional movement correctly at that moment.

@lluisrojass
Copy link
Contributor

@amio with angles we can derive intent without caring about the magnitude of the x/y values.

@amio
Copy link
Owner

amio commented Oct 8, 2019

+1 on angle. Currently we are guessing intent by angle, continuously. I was thinking if we should identify a touch should be ignored on first move.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants