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 error in touchmove event #145

Merged
merged 3 commits into from
May 23, 2019
Merged

Conversation

maurispalletti
Copy link

Fix issue #128 .

e.cancelable validation added to e.preventDefault(), to avoid console error Ignored attempt to cancel a touchmove event with cancelable=false, for example because scrolling is in progress and cannot be interrupted..

Copy link
Collaborator

@hartzis hartzis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @maurispalletti for the PR. I'd like to take some time and think about this a bit.

I don't quite know if this is a good or bad addtion or even needed.

Mainly since the callback provides the full event now, users could just check for event.cancelable themselves then call preventDefault() when they deem appropriate.

Example PR of this is the upgrade i made for react-image-gallery:

@@ -210,16 +211,17 @@ function setupGetMountedComponent(TYPE, mockListeners = mockListenersSetup) {
it('calls preventDefault when onSwiping is present', () => {
const onSwiping = jest.fn()
const preventDefault = jest.fn()
const cancelable = jest.fn()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 mind updating this and the other test usage to boolean's? const cancelable = true?

https://developer.mozilla.org/en-US/docs/Web/API/Event/cancelable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, my bad. I'll change it now and update the PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR updated!

@hartzis hartzis self-assigned this May 21, 2019
Copy link
Collaborator

@hartzis hartzis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the tests.

Still contemplating this change though.

Since you made the PR i'm assuming you are in favor of react-swipeable performing this check, can you think of any down sides? or potential issues?

@maurispalletti
Copy link
Author

@hartzis, I haven't found any potential issues. It just helps to avoid that console error.
I think it this way: If the event is not cancellable, not checking cancellable prop will just throw an error because event.preventDefault() can't be executed. With this validation, event.preventDefault() won't be executed anyway, and without throwing an error.

@hartzis
Copy link
Collaborator

hartzis commented May 23, 2019

@maurispalletti I like your thinking! I'm also very comfortable with browser support.

Let's merge and publish this.

@hartzis hartzis merged commit b95eab7 into FormidableLabs:master May 23, 2019
@github-actions github-actions bot mentioned this pull request May 18, 2023
@paulmarsicloud paulmarsicloud mentioned this pull request Jun 1, 2023
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