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
Add onDragStart, onDropStart & onDropEnd handlers #17
Conversation
src/Dnd__DndManager.re
Outdated
[@react.component] | ||
let make = | ||
( | ||
~onDragStart: option(unit => unit)=?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my specific use case, I'd want to know which item is being dragged/dropped. Could these callbacks pass the itemId
as an argument at a minimum? I could see the other data (containerId
, etc.) being useful in some cases as well (for instance, if you wanted to update the styling of a container in response to an item being dragged out of or into it). I don't need anything other than itemId
myself, but if you had to add the other stuff later it'd be a breaking change, so maybe it's worth including it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these callbacks pass the itemId as an argument at a minimum?
Totally possible, will do.
for instance, if you wanted to update the styling of a container in response to an item being dragged out of or into it
This wouldn't be useful in this callback for this use-case since it gets fired only on drag start but not when container gets changed. So it would require some special handler.
Let's proceed with the current set of handlers for now if it's enough for your current use cases. And add others as use-cases appear to figure proper api and try it out on the real world app before changing public interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's done. Please, take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this looks great! Thanks so much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np! released in 1.1.0
Fixes #16