canDrop = false not working on tree root element about pragmatic-drag-and-drop HOT 5 CLOSED

@alexreardon the jsdoc changes certainly help with understanding how the can drop check searches upwards so I think it's a worthwhile change.

👍. I'll ship them asap.

I'm still confused though as to why I can drop a 'field' type at the root level, when the root 'canDrop' is returning false.

I have been thinking about this. It think it does make sense for most cases that a disabled parent also disables all children (eg like how a disabled <fieldset> element will also disable children). However, it is nice to have the flexibility for each drop target to be able to decide whether it should be enabled or not.

A few potential options:

• Make it so that a disabled parent always disables all children
• Make it so a disabled parent disables children, unless a child states it wants to be disabled. This is problematic as the default canDrop() behaviour is true. I don't think we should distinguish between explicit canDrop: () => true and a default canDrop: () => true as that adds extra hidden meaning to the API
• Keep the current behaviour (most flexible, but perhaps a bit of friction in that folks might think that a disabled parent should disable children).

I want to think about this more and gather more feedback. Let's move this to a Github "discussion" #45.

I think the tree case of "reorder-above" is a special case. "reorder-above" is simply a string returned from the tree hitbox.

If you want to not allow "reorder-above" you have some options:

• use the block argument for attachInstruction
• locally remap the string "reorder-above" to "make-child"
• use your own hitbox function (ours is not heaps of code) which more accurately reflects your tree.

I'll close this issue for now and we can continue the broader discussion on #45.

Thanks @buzzie-bee

from pragmatic-drag-and-drop.

canDrop only blocks dragging on the current drop target, and will not block dropping on child or parent drop targets. If you want to block dragging on all children drop targets too, they too will need to return false from their canDrop(). Hopefully this understanding unblocks you. If you are leveraging something like react context, then you could pass a boolean down to all children (or pass down function that returns whether dragging is allowed).

I think we could improve the clarity of our documentation on this point.

Here is our current guidance about canDrop()

canDrop?: (args: GetFeedbackArgs) => boolean is used to conditionally block dropping. When looking for valid drop targets, @atlaskit/pragmatic-drag-and-drop starts at the deepest part of the DOM tree the user is currently over and searches upwards for valid targets. If a drop target blocks dragging (canDrop() returns false), then that drop target is ignored and the search upwards continues. canDrop() is called repeatedly while a drop target is being dragged over to allow you to dynamically change your mind as to whether a drop target can be dropped on. canDrop() being called repeatedly allows you to change your mind about whether a drop target can be dropped on after it has been entered into. This could be helpful in a situation where you are waiting on some permission information from a backend service.

• I think we could make these docs even clearer that a drop target blocking dropping with canDrop() does not impact the ability of any child / parent drop target to accept a drop
• Improve clarity of jsdoc

from pragmatic-drag-and-drop.

Work in progress new docs:

canDrop?: (args: GetFeedbackArgs) => boolean is used to conditionally block dropping on a drop target. Returning false from canDrop() will not block dropping on parent or child drop targets. All drop targets that want to not allow dropping, need to return false from their canDrop() function.

When looking for valid drop targets, a lookup starts from the deepest part of the DOM tree the user is currently over and searches upwards for valid targets. If a drop target blocks dragging (canDrop() returns false), then that drop target is ignored and the search upwards continues.

canDrop() is called repeatedly while a drop target is being dragged over to allow you to dynamically change your mind as to whether a drop target can be dropped on (including after it has already been entered into). This could be helpful in a situation where you are waiting on some permission information from a backend service.

Work in progress jsdoc

/**
   * Used to conditionally block dropping.
   * By default a drop target can be dropped on.
   *
   * Return `false` if you want to block a drop.
   * 
+  * Blocking dropping on a drop target will not block
+  * dropping on child or parent drop targets.
+  * If you want child or parent drop targets to block dropping,
+  * then they will need to return `false` from their `canDrop()`
   *
   * `canDrop()` is called _repeatedly_ while a drop target
   * is being dragged over to allow you to dynamically
   * change your mind as to whether a drop target can be
   * dropped 

Do you think that clears things up @buzzie-bee?

from pragmatic-drag-and-drop.

@alexreardon the jsdoc changes certainly help with understanding how the can drop check searches upwards so I think it's a worthwhile change.

I'm still confused though as to why I can drop a 'field' type at the root level, when the root 'canDrop' is returning false.

Is it because the canDrop of the first 'section' is being triggered and returning true, even though the instruction is 'reorder-above' which implies it's being dropped on the parent of the 'section' which is the root?

I would think that a 'reorder-above' instruction would check the canDrop of the parent of the target element for that instruction.

Edit: Your comment definitely helped me figure out what exactly was going on and get a fix working.

I added this to the items dropTargetForElements.getData function:

if(item.type === 'section' && source?.data?.type === 'field' && level === 0) {
  block.push('reorder-above', 'reorder-below');
}

Which means that the section items will block 'reorder-above/below' when they are at the root level. I still feel that the 'reorder-above' instruction not checking the canDrop of the parent is potentially problematic behaviour.

from pragmatic-drag-and-drop.

We are currently merging these docs and jsdoc improvements. They should be released within ~12 hours

from pragmatic-drag-and-drop.