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 firefox event issue related to Button tag #544

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Fix firefox event issue related to Button tag #544

merged 2 commits into from
Aug 21, 2019

Conversation

benadam11
Copy link
Contributor

This is the same issue as #118 but it also applies to button on FF

Example of issue: https://84r5vjm2yl.codesandbox.io/ (FF only)

This is the same issue as #118 but it also applies to button on FF
Remove the includes syntax in favor of logical operators

Co-Authored-By: Claudéric Demers <[email protected]>
@clauderic clauderic merged commit 6c7a90d into clauderic:master Aug 21, 2019
clauderic pushed a commit that referenced this pull request Aug 21, 2019
@clauderic
Copy link
Owner

clauderic commented Aug 21, 2019

Actually upon further investigation I actually don't think this is the same issue as #118. I believe the issue you're pointing out isn't actually specific to Firefox, and rather is just the default implementation for shouldCancelStart, see the default implementation here: https://github.com/clauderic/react-sortable-hoc/blob/master/src/SortableContainer/defaultShouldCancelStart.js#L10

Most people want their buttons to still be interactive and not cause a sort event to begin, so by default react-sortable-hoc prevents sort events from being triggered on button elements. You can fork the default shouldCancelStart implementation and define your own that you can pass as the shouldCancelStart prop of your enhanced SortableContainer component.

I'm going to be reverting this PR

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