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 duplicated instances when dragging sortable with sortable childs #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

james-em
Copy link

@james-em james-em commented Apr 29, 2024

Code pen demonstrating the bug is fixed: https://codepen.io/James-St-Pierre/pen/ZEZZqew

Closes
#16 (Lots of good information here)
#8
stimulus-components/stimulus-components#60

Explanation:

When you have a draggable list that have children with draggable list, it means both the parent and child have data-controller="sortable"

What happens when you drag around the parent list item, SortableJS moves the item using appendChild or insertBefore or other means that do not disconnect any connected event handlers. However, this seems to trigger Stimulus connect/disconnect callback on the child as they are dragged on so this library destroy and re-instantiate a new sortable instance everytime while this is unnecessary. Calling .destroy() on the child sortable while dragging the parent seems to stop the dragging process and that is what users are reporting

Drawbacks

If really an element was actually removed while dragging an item, we wouldn't destroy the SortableJS instance.

It reallty

We could lower the risk by checking if the disconnected item is actually a child of the dragged item (Sortable.dragged) however that wouldn't actually mean with 100% certainty the element is being removed.

Another thing that could be done is maintaining a map of all elements and have a setInterval(...) or something that checks every x seconds if elements are still present in the DOM and after a few consecutive ticks destroy the instance. Not sure what happens if this happen while dragging a non-related item.

A third solution could be listening to turbo:load to get rid of all previous instances.

@james-em
Copy link
Author

james-em commented May 8, 2024

@guillaumebriday Any chance you can have a quick look? :)

Copy link

@MiltonRen MiltonRen left a comment

Choose a reason for hiding this comment

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

I found that disconnecting throws errors when I'm on mobile. This would fix the errors in my environment:

    if (!Sortable.active && typeof(this.sortable) != "undefined") {
      this.sortable.destroy()
    }
    this.sortable = undefined

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.

None yet

2 participants