Shopify / draggable

The JavaScript Drag & Drop library your grandparents warned you about.
https://shopify.github.io/draggable
MIT License
18.02k stars 1.09k forks source link

Sortable breaks react-dnd when used in the same document #210

Open jarben opened 6 years ago

jarben commented 6 years ago

We are using react-dnd as well as draggable, the issue is that react-dnd stops receiving dragging events after Sortable is created in the same document. Here is a demo on codepen: https://codepen.io/jarben/pen/mxvaVE

Note that a - b - c - d items can't be dragged after calling:

const sortable = new Draggable.Sortable(document.getElementById("draggable"), {
  draggable: 'li'
});

react-dnd works fine after this code is commented out - https://codepen.io/jarben/pen/QmYzYJ

This might potentially affect other libraries using same events, is there any way to use drag events without affecting other libraries? Or any workaround you can think of?

Thanks!

ncovercash commented 6 years ago

Perhaps the issue lies in https://github.com/Shopify/draggable/blob/9f54956d24da310f7c002682fd120738096c6a7c/src/Draggable/Sensors/MouseSensor/MouseSensor.js#L189 ?

I would try with another form of input (like touch) but it doesn't seem that react-dnd supports this.

However, why not use draggable for the same functionality? I don't see why you'd need to use two separate libraries if one solves both requirements.

jarben commented 6 years ago

Thanks for pointing this out @smileytechguy , just wonder why it's bound to the whole document. Maybe specifying drag root in options would help?

There are multiple reasons to use react-dnd (or any other code using native drag events!), one is that we have a lot of code already using react-dnd so refactoring it would burn a lot of time. Another is that react-dnd just works really well with React (wraps components, unidirectional data flow,..) so it's perfect for simple source -> target dragging and data exchange in react..

We just use draggable in a vanilla ES6 component which is then imported in the the main app. The issue is that draggable is silencing any drag events which I think can be issue for various reasons...

Anyway, thanks for you thoughts again..

ncovercash commented 6 years ago

I'm not too versed in the internals of draggable itself, but I think the issue may be in that line. I don't currently have access to a way to test omitting that line, but that could be the cause of the issue. I was only proposing that you use draggable for everything as a future goal, as it is unwise to use extra libraries.

tsov commented 6 years ago

Thanks for reporting @jarben , @smileytechguy seems to have identified the source problem 👍 We need to prevent native drag events, for elements that are draggable by default (e.g. images), but there is certainly no need for attaching this on the document. Instead we'll need to check for the closest draggable element from the target and prevent dragstart or even scope it to the container instead of document.

I'll put something together soon, should be a quick fix.

@jarben I also noticed, in the codepen, that the containers you are passing into Sortable are not direct parents to the elements you declared as draggable. Sortable is the only module that requires draggable elements to be direct children of the containers you specify. Maybe we should check in Sortable#constructor and throw an error if that is not the case, additionally the Sortable/README.md should mention this too.

Hope this helped

jarben commented 6 years ago

Thanks @tsov for looking at this, closest draggable sounds like a good solution!

Also thanks for pointing out the direct parent issue, fixed in both pens.

Thanks!