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/SortableJS/Sortable/issues/1539 #1540

Closed
wants to merge 4 commits into from

Conversation

efattal
Copy link

@efattal efattal commented Jun 14, 2019

When Array.prototype is override, Sortable breaks on drag-dropping when looping on Arrays with for...in (like _detectNearestEmptySortable on sortables).

Proposed solution: loop on Arrays with forEach instead of for...in

@efattal
Copy link
Author

efattal commented Jun 14, 2019

Added some fixes and prioritize usage native cloneNode instead of jQuery.

@efattal
Copy link
Author

efattal commented Jun 19, 2019

Hi, is there any chance that this PR is accepted in near future? I need to know if I can use SortableJS in my legacy project. Thanks for letting me know.

@owen-m1
Copy link
Member

owen-m1 commented Jun 19, 2019

Sorry for the delay, I will get to this soon

@efattal
Copy link
Author

efattal commented Jun 19, 2019

Great, thanks !

@owen-m1
Copy link
Member

owen-m1 commented Jun 21, 2019

@efattal Why the change in the cloning?

@efattal
Copy link
Author

efattal commented Jun 21, 2019

For performance reason. Shouldn't available native methods be used instead of polyfills?

Edit ; however, jQuery's method copies the events, so maybe this is not à good idea.

Edit 2 : btw, those references specifically to Polymer and jQuery seem a bit weird to me.

@owen-m1
Copy link
Member

owen-m1 commented Jun 21, 2019

@efattal I think they are necessary as clearly people in the past wanted to clone the events if that code was added in the first place. Also, with prioritizing native cloning you are making the jQuery cloning clause redundant as it will never be called. This change would probably break a few use cases, so if you can please change it back then I can merge this PR.

@efattal
Copy link
Author

efattal commented Jun 21, 2019

Hi @owen-m1 , I rollbacked my change of the cloning method. Thanks for accepting the PR.

@owen-m1
Copy link
Member

owen-m1 commented Jun 23, 2019

I decided to do it myself with every loop, including plugins, in commit 854fed7.
Thank you @efattal for bringing this issue to my attention.

@owen-m1 owen-m1 closed this Jun 23, 2019
@efattal
Copy link
Author

efattal commented Jun 24, 2019

Hi @owen-m1, when could this fix be available? Thanks

@owen-m1
Copy link
Member

owen-m1 commented Jun 27, 2019

@efattal Should be available now in 1.10.0-rc3

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