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

Add callback on swipe events #366

Merged
merged 4 commits into from
Oct 23, 2019
Merged

Add callback on swipe events #366

merged 4 commits into from
Oct 23, 2019

Conversation

willedanielsson
Copy link
Contributor

This PR adds the following three props: onSwipeStart, onSwipeEnd and onSwipeMove. These functions are not required and can be added in order for the user to handle the events fired from react-easy-swipe during gestures.

A concrete example of this is related to the issue #365. On iOS physical devices, disabling the scroll of the body needs some extra configuration rather than event.preventDefault() and these functions can then be utilised in order to turn of body scrolling during swiping.

@leandrowd
Copy link
Owner

Hi @willedanielsson, thanks for the contribution, I appreciate it. However, I don't think this is the best way to solve the issue you are describing. Instead of delegating to consumers to solve this on their side, I believe we should solve in the carousel so everyone benefits from it out of the box. What do you think?

@willedanielsson
Copy link
Contributor Author

Hi @willedanielsson, thanks for the contribution, I appreciate it. However, I don't think this is the best way to solve the issue you are describing. Instead of delegating to consumers to solve this on their side, I believe we should solve in the carousel so everyone benefits from it out of the box. What do you think?

I mean the issue is only one example and it's better to allow the users to be able to use the functionality in react-easy-swipe. Maybe we want to make the background of the body red when we swipe or want to track these events, this PR would make it easier for that and is an improvement since it only adds control over the library 👍

@Jennings824
Copy link

Jennings824 commented Oct 14, 2019

Can we get this merged? I need to be able to determine user swipe events versus onChange (don't care about autoplay onChange events) for my application. The onSwipe functions added will do exactly what we need.

@sezna
Copy link
Collaborator

sezna commented Oct 18, 2019

This looks good to me, and I think these callbacks could be valuable to consumers for many reasons, even if the primary issue at hand is not fixed within the component. @willedanielsson could you resolve the conflicts?

@sezna sezna added the awaiting response A comment that is awaiting a response from the issue submitter. label Oct 18, 2019
@willedanielsson
Copy link
Contributor Author

@sezna Thanks and done 👍

@sezna sezna removed the awaiting response A comment that is awaiting a response from the issue submitter. label Oct 18, 2019
@sezna
Copy link
Collaborator

sezna commented Oct 18, 2019

I'm going to wait a little bit to give @leandrowd a chance to object just in case.

@willedanielsson
Copy link
Contributor Author

@sezna Will you merge it as well or is it @leandrowd that does it? 👍

@Igor-Palaguta
Copy link

@willedanielsson
Please can you share code how to prevent body scrolling during carousel swipe. stopPropagation and preventDefault in onSwipeXXX callbacks did not help

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.

5 participants