Refactor Draggable to decouple drag handle from the DOM node being dragged

[oandregal]

Related #7114
Makes #8764 obsolete

This PR refactors the internals of drag-and-drop, to make easier changing the drag handle in next iterations. Note that the external behavior of the drag-and-drop event is not changed in this PR.

The Draggable component

It refactors the Draggable component to decouple the drag handle (the wrapped component) from the element being dragged. After this change, Draggable is no longer be concerned with creating an actual DOM node that serves as drag handle, but the drag handle is the wrapped component itself. This makes Draggable reusable in use cases other than making a block draggable.

The existing behavior is deprecated and planned to be removed on the next version.

How to use it:

import { Dashicon, Draggable, Panel, PanelBody } from '@wordpress/components';

const MyDraggable = ( { onDragStart, onDragEnd } ) => (
	<div id="draggable-panel">
		<Panel header="Draggable panel" >
			<PanelBody>
				<Draggable
					elementId="draggable-panel"
					transferData={ { } }
					onDragStart={ onDragStart }
					onDragEnd={ onDragEnd }
				>
				{
					( { onDraggableStart, onDraggableEnd } ) => (
						<Dashicon
							icon="move"
							onDragStart={ onDraggableStart }
							onDragEnd={ onDraggableEnd }
							draggable
							/>
					)
				}
				</Draggable>
			</PanelBody>
		</Panel>
	</div>
);

export default MyDraggable;

Test

Drag and drop a block and test that it works as used to be (the block being dragged is shown as a grey box, the dragging event shows an image that is a clone of the block being dragged).

[oandregal]

Note that this code has been ported with no substantial modifications from the Draggable component.

[oandregal]

Do we need the components-draggable class?

[oandregal]

Note that we need to make sure onDragStart is executed after initDragging.

This is due to how Draggable and Drag and Drop behavior was originally set up. The isDragging prop takes its value from state.dragging which is changed in the onDragStart and onDragEnd methods of the block. Upon isDragging being true, the visibility of any block's children is changed to hidden, making them un-reactive to events (so the initDragging handler ends up not being set up).

In the next iterations, when the drag handle doesn't have a dedicated DOM node but uses an existing component, this hack should be gone.

[gziolo]

Technically this PR isn't a breaking change, but it is going to be once the deprecation is removed.

[youknowriad]

Great work and I can see how decoupling the rendering of the DragHandle is a good idea. Though, there's something bothering me about the API of this higher order component. I don't like that the HoC provide us with a initDragging component where it's the responsibility of the inner component to call onDragStart but it automatically handles onDragEnd somehow.

What I propose is to have this:

const DragHandle = ( { onDragStart, onDragEnd } ) => {
  return (
    <button { ...{ onDragStart, onDragEnd } }>Drag Handle</button>
  );
}

export withDraggable(( props ) => { 
   transfertData: props.data,
   clonedElement: props.clonedElement
} )

But if you take a look at this (above), it's the exact API as the Draggable component we have right now, only difference is that the drag handle is rendered by the consumer and not the Draggable component itself which mean we can do this:

<Draggable transfertData={ data } clonedElement={ element }>
  { ( { onDragStart, onDragEnd } ) => (
    <button { ...{ onDragStart, onDragEnd } }>Drag Handle</button>
  ) }
</Draggable>

And this "backwards compatible" because we can check that if children is function we apply the behavior proposed here, if not we just keep rendering the handle in Draggable. At the same time we can add a "deprecated" call when children is not a function.

Thoughts?

[oandregal]

@youknowriad I like that! It also solves some concerns I had regarding the API leaking some important details. See 6e4fd6f and 1185662 messages.

Implemented and pushed that API change. I can modify docs later, once we agreed to the API.

[youknowriad]

I think we can reduce the changes required to implement that if we just add the "children as function" support to the current Draggable component.

[oandregal]

I'm open to either use the withDraggable HOC or refactoring Draggable to use the children prop as a render prop. Reasons I'd lean towards keeping withDraggable:

• it makes for a simpler API
• per consistency with how we deal with other events (see higher-order directory)
• a HOC seems to communicate better that this needs to be passed a single child

[oandregal]

@youknowriad We commented at the same time :) What do you think of #9311 (comment) ?

[oandregal]

I think we can reduce the changes required to implement that if we just add the "children as function" support to the current Draggable component.

I totally understand this. However, I'd rather reframe the debate to optimize what's best from an user perspective (coherence with the rest of the codebase, easiness of use, etc). I'm undecided, and would love to hear your thoughts about why a render prop in Draggable would be best than the withDraggable HOC in this case.

[youknowriad]

it makes for a simpler API

I'd argue that the HoC is always more complex to understand than a simple component. Granted that children as function is not "simple" but I personally think it's easier to understand than a function prop =>( { transferData, clonedElement} )

per consistency with how we deal with other events (see higher-order directory)

We already use Render Props in MediaUpload, Dropdown and Fill (I'm probably also missing other use-cases).

a HOC seems to communicate better that this needs to be passed a single child

You can wrap the whole component in a HoC and only pass the injected props to a single element inside this component. I'd say that the disconnect between the place you apply the component and the element where the props are applied is more "obvious" when using HoCs (just look at the way you refactored BlockDraggable.

---

I'm not saying the render prop is always superior to an HoC and I guess it can mostly be considered a personal preference, my personal preference in this particular use case though is towards a render prop. Because:

1- We already have a component <Draggable> which will stay backwards compatibility and the API changes are minimal.

2- We can have a default render function which means if we don't want to control the rendering of the draggable handle we can just drop the children entirely.

[oandregal]

@youknowriad Pushed the necessary changes to use Draggable and remove any traces of withDraggable.

I took a step back and considered the API with fresh eyes. I think now the data flow is clearer: Draggable takes the onDragStart / onDragEnd props passed to the component (if any) and injects onDraggableStart / onDraggableEnd to the drag handle which in turn needs to bind them to its own event callbacks. The use of HOCs is so pervasive in Gutenberg that at some point I got used to the indirection they introduce. Your rationale has been very helpful, thanks!

[oandregal]

Is there a better way to explain this change?

[gziolo]

This deprecation will be removed in 4.0 release if it is planned for 3.8 release.

[oandregal]

Do we need to include the version that will remove the behavior here? I haven't seen it anywhere else.

[oandregal]

Just to make sure: the deprecation is added to the version it was deprecated, right? Or should we move this to the 4.0 section?

[gziolo]

The current deprecations are listed below and are grouped by the version at which they will be removed completely.

It should be under 4.0 😄

[oandregal]

✍️ 32006d3

[dsifford]

FYI @nosolosw: the draggable attribute is enumerated, so shorthanding it like you have here is actually not valid.

See here: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/draggable

cc @youknowriad

[youknowriad]

Isn't this something already taken care of by React? we should fix if not :)

[oandregal]

@dsifford you're right in that draggable is enumerated. We could probably the example (and code) to remain as true to the HTML standard as possible. However, note that React does the conversion for us.