atlassian / react-beautiful-dnd

Beautiful and accessible drag and drop for lists with React
https://react-beautiful-dnd.netlify.app
Other
33.13k stars 2.52k forks source link

Discussion: placement of handles and flow of data #498

Open spatialvlad opened 6 years ago

spatialvlad commented 6 years ago

I really like the great library that you guys have created. Our team is currently considering the option to start using react-beautiful-dnd instead of react-sortable-hoc we currently use.

However, it looks like there is a blocker for adopting your library for us due to inability to pass state/callback from Draggable into the onDragEnd of the DragDropContext. I hope I just missed the option in the documentation, but I did make a thorough search before opening the issue.

Your documentation recommends wrapping whole application with a DragDropContenxt. For most real-life production applications it means that DragDropContext will be multiple levels of hierarchy away form the Draggables. Even if multiple DragDropContexts are used, it is still very likely that the DragDropContext (probably with Droppable) will be not in the same file/component as the Draggables.

It is definitely not a big issue to create something like a top-level reducer inside of the onDragEnd that first filters results based on type, then applies corresponding logic, getting needed data from the Redux store. And this solution will work in some cases.

However, there are several drawbacks and even blockers with this approach:

It is extra work to get the Redux store data and process it in the corresponding onDragDrop reducer. This work was already probably conducted at the component level - Draggable or Droppable. And simply passing it would be much easier than trying to do it again. Much bigger problem is that not all Component state lives in Redux store. This is a quite common and even recommened (e.g. https://github.com/reactjs/redux/issues/1287 ) behavior. This means that onDragEnd currently simply does not have a way to get needed state, unless substantial refactoring of existing code is done to put all needed state in a Redux store (and even then it might be not convenient).

Other libraries do not seem to have same limitation, as both offer a callback on the Draggable level to run after dragging ends.

E.g. React DnD offers endDrag for the DragSource. React-sortable-hoc has onSortEnd for the SortableComponent.

Your library looks superior to the above mentioned libraries in many aspects, but current limitation of not being able to define/pass a onDragEnd callback or at least some state from the Draggable is unfortunately greatly undermining its usability, especially if we are talking not about greenfield projects, but ones already developed and have lots of (presentational) state on the components level.

I hope I just missed some piece of the documentation and you can direct me where to have a look.

I did find something that seems related (but still does not seem to answer my question): https://github.com/atlassian/react-beautiful-dnd/issues/120 https://github.com/atlassian/react-beautiful-dnd/issues/466

It would be really great if there actually was some way to pass a callback from Draggable. I would appreciate if you could help me find out how to do this.

Thanks.

alexreardon commented 6 years ago

Thank you for the thought-provoking issue. The core issue is that there is no clear recommendation for rolling up lots of lists into a single onDragEnd. It is difficult to make a recommendation as this library has no opinions about how you manage your state. Perhaps some additional hooks are a possible approach. There may even be some sort of nesting context approach that could work.

We have aimed to keep the api as light as possible. The understanding is that if we return the id of the things that have been dragging then a consumer should be able to look up whatever data they need from that id. If that data is unavailable then that is a separate issue that needs to be managed by your implementation.

A 'hack' to pass data now is to json encode your data as your id. Then you will get a json string as the id in your onDragEnd that you can parse. That is a hack, but it might be enough for you for now.

I will no open up the api to allow direct passing of data until we explore this problem more

spatialvlad commented 6 years ago

Thank you for your prompt reply to the issue and offering the ‘hack’.

I’ll discuss it with my team to see if they will be willing to adopt this approach, as it is still quite a bit less convenient compared what the currently library we are using is offering.

I really like that you are trying to keep the api of the library as light as possible. And it is great that as you say it “this library has no opinions about how you manage your state”.

My understanding that the core issue is that while react-beautiful-dnd does not have opinions about how state is managed, it is based on an assumption that all needed state is easily available to the onDragEnd either via a (redux) store, or via its placement in the same file as Draggable.

As I indicated above, it is often not the case, as it is common in the React/Redux world to have some state inside the components, not Redux. And big code-bases are very unlikely to have DragDropContext and Draggable in the same file.

Additional hooks/callbacks seem to be a good approach to solve the problem.

Anyway, thanks again for putting so much effort in this great library. I think that resolving the issue will help a much wider adoption of the library.

Frozenlock commented 6 years ago

I prefer to add functions inside the ID, rather than using json encoding. In my case, I add source-fn and destination-fn to handle what is happening to the source droppable and destination droppable. Those functions are then extracted and called by the on-drag-end callback.

Those functions capture a reference to the state at level of the droppable component. This means that the DragDropContext is completely independent from the droppable components.

alexreardon commented 6 years ago

Interesting @Frozenlock. Does that work with our current setup?

Frozenlock commented 6 years ago

It should work. (I'm still on version 6.)

The only caveat is that I'm using Clojurescript. The principle should be the same however : store functions inside IDs and call those functions with the on-drag-end callback.

alexreardon commented 6 years ago

Very interesting. We currently say it needs to be a string, but I do not think we do any string operations on the values. As long as referential equality is maintained it could work. This gives some great food for thought. Thanks @Frozenlock !

alexreardon commented 6 years ago

I need to work through whether that would enable people to get done what they want to. Using a function is a brilliant idea

Frozenlock commented 6 years ago

Playing a bit around (with multiple droppables this time), here's what seems to work inside the droppable ID:

Maps do not work.

Maps will be passed to the on-drag-end callback as expected, but things break when multiple droppables exist. (The equality test must break with maps...)

Here's a cool hack however : An array with identifiers AND a map. The identifiers will insure multiple droppables are not considered the same, and all sorts of goodies can be stored into the map to extract in the on-drag-end.

alexreardon commented 6 years ago

Thanks for looking into this @Frozenlock. Glad to hear there is an option for people to use today. I think it would be a good idea to formalise this and provide a better first class api - as simple as an extra prop on a draggable that is passed on all the hooks which could contain anything - data or a function.

For now it looks like users can hack the id to achieve this. Keep in mind though that this might have unexpected bugs as we assume the id is a string. Also, the id needs to be a unique identifier for the item so you will need to ensure that referential equality is never the same for multiple draggables

alexreardon commented 6 years ago

Adding a prop with a name like:

to pass around to the hooks seems like a good idea.

Not sure what to name the prop though

Frozenlock commented 6 years ago

extra or params convey the intention pretty well I think.

This field would be available at the top level (with the draggableId), and also inside the source and destination fields, allowing the user 3 different entry points for extra data. Is that it?

alexreardon commented 6 years ago

Details to be confirmed, but something like that.

Frozenlock commented 6 years ago

Upgraded to 7.x.x, doesn't work anymore. :cry:

Warning: Failed child context type: Invalid child context private-react-beautiful-dnd-key-do-not-use-droppable-id of type array supplied to Droppable, expected string.

Hopefully the new field will make its way in the next version.

davps commented 6 years ago

Thank you guys for the ideas you shared on this subject. Based on it, I decided to pass stringified objects. The onDragEnd function arguments structure the information by draggable and droppables for source and destination, so, I followed the same pattern. The example below shows how I am using it:

The droppableId property of my <Droppable> source looks like this:

droppableId={JSON.stringify({
    id:'droppable_people', 
    table: 'people'
})}

The droppableId property of one of my <Droppable> destination looks like this:

droppableId={JSON.stringify({
    id:team.id, 
    table: 'team'
})}

Passing the objects (stringified in this case) is useful to manage my hierarchies since I have different kinds of draggables, for example, the draggableId of my <Draggable> looks like this:

draggableId={JSON.stringify({
  id:people.id, 
  table: 'people'
})}

or like this:

draggableId={JSON.stringify({
  id:task.id, 
  table: 'task'
})}

so on my onDragEnd function handler of my <DragDropContext> I parse the objects:

const params = {
  droppable: {
    source: JSON.parse(source.droppableId),
    destination: JSON.parse(destination.droppableId) 
  },
  draggable: JSON.parse(result.draggableId)
};

and use it as needed, for example, on those conditions:

if(params.droppable.source.id === 'droppable_people' && 
    params.droppable.destination.table === 'team' )

and

if(params.droppable.source.table === params.droppable.destination.table)
saiichihashimoto commented 6 years ago

I think we're going about this the wrong way. It's good that "this library has no opinions about how you manage your state" but I'm finding that, in most cases, it's in the context of Droppable that I know what I want to accomplish, not at a much higher context of DragDropContext. It's too detached.

The current suggestions are to pass up state from the Draggable to the DragDropContext. But the fact that you're mapping everything back to the Droppables seems to indicate that the logic is far from the relevant area of responsibility. With enough types and Droppables and the fact that all of that gets handled in onDragEnd, you're bound to have complex logic even though you've most likely pulled the same state at the relevant Droppables.

Here's my suggestion. We leave onDragStart, onDragUpdate, and onDragEnd for global UX concerns like blocking updates on drags and backwards compatibility. Droppables get a onDroppedOnto and some kind of "leave" hook (onDroppedFrom?). They'd receive what onDragEnd receives now, more or less.

I think this solves most use cases. onDragEnd doesn't have to get complicated because the Droppables will receive the events that matter. Since the Droppable most likely handles retrieving, removing, persisting, sorting, etc from state, it's the right place to change the state from the drag.

Also, I think passing information about the relevant Draggable will become much less necessary. "The understanding is that if we return the id of the things that have been dragging then a consumer should be able to look up whatever data they need from that id." I think you're right but we're already looking up that data at the Droppables. The Droppables will need the state to have generated the Draggables in the first place, so it's the right place to look things up by id.

Here's my super hack to accomplish this for myself:

alexreardon commented 6 years ago

Super interesting @saiichihashimoto. Thank you

ianstormtaylor commented 6 years ago

First off, thank you for this great library. It's really well architected.

I just wanted to +1 the approach @saiichihashimoto advocates. I think that both Droppable and Draggable should be allowed to define the onDrag* handler methods themselves, so that we can handle state changes right where they need to be handled in the component hierarchy.

For Redux-driven apps the current top-level DragDropContext makes a lot of sense. But for apps using any render-prop-based APIs to manage state, we need be able to define drag/drop handlers inline where the state actually lives. For example in components that are rendered with react-apollo, or REST alternatives like holen or react-request.

(This also applies to apps using HOC-style state management libraries like react-refetch, or even just the built-in state/setState primitives React provides.)

For example, an app might be structured something like:

<DragDropContext>
  <App>
    ...
    ...
    <Query query="some list query">
      {({ list, updateList }) => (
        <Droppable ...>
          {data.map(item => {
            <Draggable ...>
              ...
            </Draggable>
          })
        </Droppable>
      )}
    </Query>
  </App>
</DragDropContext>

In this case, the list of items and the updateList method that is used to synchronously update the state are only available at the level of the tree where the droppable is defined.

To update them from the DragDropContext.onDragEnd requires lots of hacks (using event emitters and refs as @saiichihashimoto mentioned) to work.

Instead, it would be ideal to be able to just define an onDragEnd on the <Droppable> container, and have it scoped to only be called when that specific droppable container is either the source or destination of a drag.

The same actually goes for <Draggable>. I have a use case in my app right now where I need to "collapse" a draggable item before its dragged, and right now doing that requires the same hacks since the expanded state is only available deeper in the render tree.


So I think that both <Droppable> and <Draggable> should be allowed to define all three handlers: onDragStart, onDragUpdate, onDragEnd. For droppables, the handlers should only be called when the source.droppableId or destination.droppableId matches the droppable. (And obviously for draggables only when the draggableId matches the draggable.)

(@saiichihashimoto suggests separating them into two handlers, like onDroppedFrom and onDroppedOnto, but I think this can be handled by simply checking the source.droppableId and destination.droppableId instead to keep things simple.)

That would make this library incredibly flexible, an unopinionated to any of the different approaches to state management.

Thanks!

alexreardon commented 6 years ago

There are some things that need to be thought through (such as what hooks are called when moving from one list to another), but I am liking the direction of this discussion. It seems like adding some callbacks could be a useful addition!

twheys commented 6 years ago

This affects me as well. I have a case where the are multiple types of "droppables" nested within one another. Since I need to use one DragDropContext for the whole thing, I need to pass some information into the onDragEnd (or potentially any other hook functions) in order to work out what to do, based one what was dropped where.

I'd think this could be implemented in a fairly straight forward way by allowing a prop to be set on the Droppable and Draggable components which gets passed in to the hook functions when they are called. I don't have a strong opinion on what this prop should be called.

type DraggableLocation = {|
  ...
  droppableId: DroppableId,
  params: object
|};

type DropResult = {|
  ...
  source: ?DraggableLocation,
  destination: ?DraggableLocation,
  params: object
|}
euroclydon37 commented 6 years ago

This would be huge. I'm cloning and modifying a list before submitting a result to a database, which will, in-turn, result in the redux store being updated. A user will modify the list via an "edit" popup. The cloned list is held in the Editor component's state and is inaccessible to DragDropContext.

I was thinking that I could pass a bound function as the id (per suggestions earlier in this thread) for updating that state, but it seems like this hack is no longer possible.

I gotta say, though, thank you for all of your excellent and creative work on this library. It's really fantastic.

Frozenlock commented 6 years ago

Whatever the path taken, I'd really appreciate if we can have the ability to pass arbitrary data (functions) around.

Aarbel commented 6 years ago

will this feature be applied in last version ?

tslocke commented 5 years ago

UPDATE - turns out this seems to be causing an intermittent bug where the library throws an error at the start of a drag operation - cannot modify draggables during a drag operation.

I'm stuck on v7.1.3 so can't say if this works with the current version, but the following sneaky hack seems to work. Provide an object as your draggableId like so:

{payload: arbitraryData, toString: () => "unique string"}

Note this doesn't work with droppableId, due to the Failed child context type mentioned above. @alexreardon maybe just loosening the prop-type for droppableId is all that would be needed to get this to work?

@Frozenlock I happen to also be doing this from ClojureScript (reagent), i.e.

{:draggable-id #js {:payload my-data :toString (fn [] "...")}}

The fact that this is a JS object means Reagent doesn't mess with the payload, so ClojureScript data comes through intact. I guess that doesn't apply to you as your payload would be a function, but worth mentioning.

alexreardon commented 5 years ago

I am super open to suggestions for API proposals for this one.

Would allowing passing a data object be enough? I am leaning towards calling it payload. Would we need to have hooks for Droppables if we had this payload?

If the payload could be type any, then it could be a function that you could do what you want with

/cc @Frozenlock @saiichihashimoto

alexreardon commented 5 years ago

A complication could be: if we allow a payload prop, then we would capture that at drag start and keep it a reference to it. We would not recollect it during a drag. Could pass this payload to onDragEnd (maybe the other hooks too?).

This could be strange if people pass in a new function during a drag - it won't get called. The old one will.

I think this is okay as long as we call it out.

Thoughts @Frozenlock ?

Frozenlock commented 5 years ago

I don't see this as a problem in my case.

This could be strange if people pass in a new function during a drag - it won't get called.

Is this an optimization issue? I can imagine cases where the payload would benefit from an update while in mid-drag.

alexreardon commented 5 years ago

I am thinking about how we perform our capturing. We currently do not recapture any information from a Draggable once the initial lift is done. Needing to recapture the payload would be a pain

Frozenlock commented 5 years ago

Ah I see. Not a big deal, especially considering there are hooks available that can do the same thing.

NunoCardoso commented 5 years ago

I like the payload proposal. I would rather have them on the onDragEnd, as I can then have a complete separate DragDropcontext component that acts as a stateless gatekeeper that checks every event and authorizes/denies actions by looking at the source, destination AND payload.

Still, if it's on onDragStart, we can just redux it, no problem. Yet, this is a very welcome change, as it avoids me to go into ref forward hell in my component hierarchy to get the Draggable elements.

Can I infer from your words that you will add this feature in your todo list?

alexreardon commented 5 years ago

The simpliest path forward would be to add a payload prop to Draggable that would be of any type. This prop would be captured at drag start and provided to onBeforeDragStart, onDragStart, onDragUpdate and onDragEnd.

It would be an any so a function could be passed in. You could then call this function in your hooks. Something like:

onDragEnd = (result) => {
   // you could pass the whole result to your payload function if you want
   result.payload(result);
}

Would this be sufficient @Frozenlock? I think this is the most flexible option. I am struggling to come up with a generic api for Droppables / Draggables given that you can move from one list to another.

onDragEnd for a Draggable: can it see the data that generated it? onDragEnd for a Droppable: which droppable do we call? The original, destination, both?

Frozenlock commented 5 years ago

I've re-read my previous comment on this issue and double-checked my current code : I'm associating functions to the droppable. The droppable itself can know how to deal with an element added/removed, while the element being dragged simply has to know its identity.

For example, let's say I drag an element from a list of 10 elements (droppable1) to a list with at most 1 element (droppable2).

One can also imagine a scenario where the dragged object needs to be "transformed" before being added to a particular list.

The draggable doesn't know how it should act when dropped on a list. This is the droppable's job.

Frozenlock commented 5 years ago

(It doesn't mean that Draggable shouldn't have a payload, simply that Droppable should have one.)

Frozenlock commented 5 years ago

Any chances this could make its way into #838? I'm still on 6.0.0 because of this issue and I'm eager to use a newer version.

tslocke commented 5 years ago

To follow up on @frozenlock’s point about payloads on droppable, I just want to clarify that a payload on the draggable is definitely what I would like to see added.

Of course, I don’t have any objection to there being something similar on the droppable, although I would suggest “payload” is not the right term in that case, as it implies something is moving and it has some contents inside.

Tom On 5 Oct 2018, 19:22 +0100, Frozenlock notifications@github.com, wrote:

Any chances this could make its way into #838? I'm still on 6.0.0 because of this issue and I'm eager to use a newer version. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

Frozenlock commented 5 years ago

@tslocke Would you mind detailing a little how you'd use the payload on the draggable? Especially considering you're using Reagent, I wonder how we ended up at opposed ends.

alexreardon commented 5 years ago

It seems like a generally useful thing to have a Draggable > payload. It could open up a few control flow possibilities, but it seems like people would like it as a mechanism to pass information to onDrag*.

Given this I think I will try to get it into #838 @Frozenlock.

Thoughts?

alexreardon commented 5 years ago
export type DragStart = {|
  draggableId: DraggableId,
  type: TypeId,
+  payload: ?mixed,
  source: DraggableLocation,
  mode: MovementMode,
|};

And Draggable props would get payload: ?mixed

This payload would be captured once for a drag, on drag start (same as draggableId)

alexreardon commented 5 years ago

I have added Draggable > payload to react-beautiful-dnd-next@10.0.0-alpha.3

I have removed it for now until we get a clearer way forward

Frozenlock commented 5 years ago

I'm still unclear about the advantages of having a payload on the draggable as opposed to the droppable. (see previous comment)

Could someone give me a control flow example with the draggable?

alexreardon commented 5 years ago

I was thinking that you could pass something into the draggable payload through context if needed. But perhaps something more robust is needed. Thinking out load:

payloads: {|
  draggable: ?mixed,
  // draggable payloads:
  source: ?mixed,
  destination: ?mixed,
|}

That way a Draggable and a Droppable could specify a payload. It just feels a little strange for a Droppable to define a payload as it does not move

I do not think a Droppable payload makes a lot of sense. But hooks on a Droppable do for the purpose of updates. It is almost as if we need a Draggable > payload as well as onDrag* hooks on Droppables. The part I have not figured out is the calling of the Droppable hooks. It is a bit confusing because often operations span multiple Droppables. Do we just call all the hooks on the Droppables if they are impacted by an update / drop? 🤔

Frozenlock commented 5 years ago

I was thinking that you could pass something into the draggable payload through context if needed

Wouldn't this only work for the source Droppable? I can easily store something from the source Droppable into the Draggable's payload, but what about the destination Droppable?

My initial approach was to store functions (source-fn and destination-fn) into the Droppable IDs because they both end up in the onDragEnd hook. In other words, it was possible for my Droppable components to provide all the manipulation functions and leave the DragDropContext as universal as possible.

Let's give a simple example with a food Draggable :

The way I see it is that each Droppable is responsible for providing its manipulation functions. By keeping everything separated, I can even decide to add a completely new Droppable without modifying any of the existing code. For example I can add a Cook Droppable.

However, considering I haven't seen many people discussing the dispatching issue, I must be missing some obvious alternate way of dispatching on the onDragEnd hook.

tslocke commented 5 years ago

After you asked for more details @frozenlock I went back and had a look at the code and was reminded that my previous comment was completely wrong : ) I put EDN stringified data on both the droppable and the draggable.

I have what I think is fairly typical for ClojureScript apps - a single big associative data structure holding the state of the app. Drag and drop allows the user to move some data from one place in that structure to another, so for each droppable I store the path to the collection (a vector of keywords and integers). On the draggable I store the path to the data itself, although that is redundant really - the path on the droppable plus the index of the draggable gives the same information.

When the user drags some data I can use get-in to find the data, and update-in to make the changes (remove from source, add to destination).

So it’s a very data-centric approach. I rarely need to run any custom code for a drag-drop operation, but if I do I have a  simple system for adding data watchers on nodes in the data, which can trigger anything (via core.async channels)

Tom

On 8 Oct 2018, 03:22 +0100, Frozenlock notifications@github.com, wrote:

I was thinking that you could pass something into the draggable payload through context if needed Wouldn't this only work for the source Droppable? I can easily store something from the source Droppable into the Draggable's payload, but what about the destination Droppable? My initial approach was to store functions (source-fn and destination-fn) into the Droppable IDs because they both end up in the onDragEnd hook. In other words, it was possible for my Droppable components to provide all the manipulation functions and leave the DragDropContext as universal as possible. Let's give a simple example with a food Draggable :

• Freeze Droppable; • Thaw Droppable.

They way I see it is that each Droppable is responsible for providing its manipulation functions. By keeping everything separated, I can even decide to add a completely new Droppable without modifying any of the existing code. For example I can add a Cook Droppable. However, considering I haven't seen many people discussing the dispatching issue, I must be missing some obvious alternate way of dispatching on the onDragEnd hook. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Frozenlock commented 5 years ago

(...) so for each droppable I store the path to the collection (a vector of keywords and integers). On the draggable I store the path to the data itself, although that is redundant really - the path on the droppable plus the index of the draggable gives the same information.

Thanks @tslocke, it appears you also store data inside the Droppables to use later. I'm not just a random crazy person! :-p

alexreardon commented 5 years ago

I am keen to get something in. However, we currently lack a cohesive path forward.

Draggable > payload seems reasonable

But what about Droppables? Should they too have a payload or should we expose hooks (eg onDragStart) on them?

What would the exact behaviour of a onDragUpdate / onDragEnd be when moving between lists?

rhys-vdw commented 5 years ago

I think this feature would solve a problem we have too, I accidentally broke our react-beautiful-dnd solution by changing the string ID used on Draggable, unaware of this workaround:

  private handleDragEnd = (result: DropResult) => {
    const { onMoveQuestion, onMoveOption } = this.props

    // Check if it was dropped outside of the list
    if (result.destination == null) {
      return
    }

    // Since we handle both question and option drags with the same
    // context we have to differentiate them by the draggableid
    const draggableId = result.draggableId.split(".")
    const dragSubject = draggableId[0]
    const sectionIndex = parseInt(draggableId[1], 10)
    const questionIndex = parseInt(draggableId[2], 10)

    if (dragSubject === "question") {
      onMoveQuestion(
        sectionIndex,
        result.source.index,
        result.destination.index
      )
    } else if (dragSubject === "choice") {
      onMoveOption(
        sectionIndex,
        questionIndex,
        result.source.index,
        result.destination.index
      )
    }
  }
saiichihashimoto commented 5 years ago

Sorry I've been MIA for a while. :-) I think this library is wonderful and I've used it in a ton of projects. But yes, the context in which I want to handle drag events could still be with the Droppable, imo. I'm starting to lose track of this conversation, so let me share my wrapper around this library I currently use.

https://gist.github.com/saiichihashimoto/2c956fac9184d881922ab636628787f6

I think this exemplifies what a few people have mentioned. It generates a unique droppableId for each Droppable. These IDs have only mattered me just to achieve what comes next:

Droppables are receiving the drop event. It receives DragDropContext.onDragEnd to have a reference to draggableId. I'm usually generating a list of Draggables in a Droppable, so I probably have react-redux or whatever I'm using to map draggableId back to my data handy.

This is where I'd rather a payload. You mostly certainly use some object to render Draggables but, at onDragEnd, you just have a draggableId. I'd rather have that same object. Whether I'm using redux, flux, whatever, I'd prefer the object that represents the Draggable, because a draggableId just means I have to go find the object again.

This would let us get back to recommended react paradigms. I think of this library as an excellent React UI/UX for passing elements between lists. We usually think of these lists getting or receiving elements rather than a global context handling all of their interactions, and having the hooks reflect that would be very intuitive. We no longer need droppableId or draggableId, Draggables just unroll from arrays (using key, most likely), and then Droppables handle when they gain/lose their elements.

Anyways, I love this library. The interactions are beautiful and solid, and I can't imagine the DOM nightmare that it's keeping away from me. I think having this would make the component management rock solid and intuitive.

JReinhold commented 5 years ago

I like @saiichihashimoto's approach, and I'm all in for adding hooks to the Droppables. It could be a single onDrop hook on the Droppable, that would fire both on the source and the destination Droppable, no matter if the Draggable is moved between different lists, etc. However it could be a problem if the hook was fired twice when a Draggable is moved within the same Droppable (the Droppable would both be the source and the destination). eg. if we need to tell the backend that our list have updated, we don't want to tell it twice.

Motivation

As @saiichihashimoto mentions, it is common to have the DragDropContext pretty far away (hierarchy wise) from the actual lists, Droppables and Draggables. What I currently do, is essentially lifting the state up, and passing it down to the Droppables using Context. So I have a parent component high up that manages the different lists, and holds the DragDropContext, thereby giving the onDragEnd access to the lists state. The list are then passed down via context to the Droppables, Which means they can't manage the lists in any way. My understanding is that this is a bit different from @saiichihashimoto's approach, where the lists are located at the Droppables, but they listen to Events that are passed from higher up the tree. But the problem is the same, we need to pass something (the actual state, or an event to change the state) from a parent component down to our list containers, instead of just giving the lists their own way of managing the lists.

I hope I at least contributed a little to the discusion. Have a nice day y'all!

saiichihashimoto commented 5 years ago

So @alexreardon, I think it boils down to:

12tp12 commented 5 years ago

@saiichihashimoto actually my workaround for this was adding a new middleware where I can get the store without connecting (to redux) the component that has the DragDropContext. it works pretty well for me.

alexreardon commented 5 years ago

A good time to make this change could be when we change the API to be hook based #871