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.

Sign up for GitHub

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

Solves "insert Before error" issue on nested lists #1784

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Solves "insert Before error" issue on nested lists #1784

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Sortable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1692,7 +1692,7 @@ Sortable.prototype = /** @lends Sortable.prototype */ {
if (Sortable.eventCanceled) return;

// show clone at dragEl or original position
if (rootEl.contains(dragEl) && !this.options.group.revertClone) {
if (dragEl.parentNode == rootEl && !this.options.group.revertClone) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (dragEl.parentNode == rootEl && !this.options.group.revertClone) {
if ( (rootEl.contains(dragEl) || dragEl.parentNode == rootEl ) && !this.options.group.revertClone) {

I have a feeling this might be safer. I'm unsure what remove the old check might do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in my opinion the change above is equivalent to the original so doesn't solve the issue.
I try to better explain my proposal:

original) "rootEl.contains(dragEl)" means dragEl is a descendent of Root so it can be child, child of child etc...the condition is too broad and results in an error when dragEl is not just a child.

modified) "dragEl.parentNode == rootEl" means dragEl is child of root. That's exactly the condition you have to check before "rootEl.insertBefore(cloneEl, dragEl)", if the condition is not met the code will simply move on to one of the following methods to find the right insertion point.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense to me

rootEl.insertBefore(cloneEl, dragEl);
} else if (nextEl) {
rootEl.insertBefore(cloneEl, nextEl);
Expand Down