Fix duplicated instances when dragging sortable with sortable childs #28
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
orinsertBefore
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 reportingDrawbacks
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.