Skip to content
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

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

Merged
merged 24 commits into from Sep 3, 2018

Conversation

Projects
3 participants
@nosolosw
Copy link
Member

nosolosw commented Aug 24, 2018

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).

@nosolosw nosolosw self-assigned this Aug 24, 2018

const cloneHeightTransformationBreakpoint = 700;
const clonePadding = 20;

const withDraggable = createHigherOrderComponent(

This comment has been minimized.

@nosolosw

nosolosw Aug 24, 2018

Author Member

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

class BlockDraggable extends Component {
render() {
const { clientId, elementId, index, initDragging, isDragging, layout, rootClientId } = this.props;
const className = classnames( 'components-draggable', 'editor-block-list__block-draggable', {

This comment has been minimized.

@nosolosw

nosolosw Aug 24, 2018

Author Member

Do we need the components-draggable class?

@nosolosw nosolosw force-pushed the add/with-draggable branch 3 times, most recently from 0e10f1e to eb5f57c Aug 24, 2018

layout,
};
initDragging( elementId, transferData )( event );
setTimeout( () => onDragStart( event ) );

This comment has been minimized.

@nosolosw

nosolosw Aug 27, 2018

Author Member

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.

@nosolosw nosolosw requested review from aduth and youknowriad and removed request for aduth Aug 27, 2018

@nosolosw nosolosw referenced this pull request Aug 27, 2018

Closed

Use the breadcrumb as the draggable handle #8764

4 of 10 tasks complete

@nosolosw nosolosw changed the title Add withDraggable HOC Refactor BlockDraggable to use the withDraggable HOC Aug 27, 2018

@gziolo gziolo added this to In Progress in API freeze via automation Aug 28, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Aug 28, 2018

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

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Aug 28, 2018

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?

@nosolosw nosolosw force-pushed the add/with-draggable branch from eb5f57c to 1185662 Aug 29, 2018

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 29, 2018

@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

This comment has been minimized.

Copy link
Contributor

youknowriad commented Aug 29, 2018

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.

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 29, 2018

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
@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 29, 2018

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

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 29, 2018

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

This comment has been minimized.

Copy link
Contributor

youknowriad commented Aug 29, 2018

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.

@nosolosw nosolosw force-pushed the add/with-draggable branch from 1185662 to e804f80 Aug 30, 2018

@nosolosw

This comment has been minimized.

Copy link
Member Author

nosolosw commented Aug 30, 2018

@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!

@@ -14,6 +14,7 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo

- `wp.components.withContext` has been removed. Please use `wp.element.createContext` instead. See: https://reactjs.org/docs/context.html.
- `wp.coreBlocks.registerCoreBlocks` has been removed. Please use `wp.blockLibrary.registerCoreBlocks` instead.
- `wp.components.Draggable` as a DOM node drag handler has been deprecated. Please, use `wp.components.Draggable` as a wrap component for your DOM node drag handler.

This comment has been minimized.

@nosolosw

nosolosw Aug 30, 2018

Author Member

Is there a better way to explain this change?

This comment has been minimized.

@gziolo

gziolo Aug 30, 2018

Member

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

This comment has been minimized.

@nosolosw

nosolosw Aug 30, 2018

Author Member

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

This comment has been minimized.

@nosolosw

nosolosw Sep 3, 2018

Author Member

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?

This comment has been minimized.

@gziolo

gziolo Sep 3, 2018

Member

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 😄

This comment has been minimized.

@nosolosw

nosolosw Sep 3, 2018

Author Member

✍️ 32006d3

@nosolosw nosolosw referenced this pull request Sep 3, 2018

Merged

Add/drag handle #9569

10 of 10 tasks complete

nosolosw added some commits Aug 24, 2018

Pass props to wrapped component
and let React bind the class methods as well.
withDraggable's event handlers take control
To avoid withDraggable's event handlers to be shadowed by the children props
we need to make sure they are passed at the end.
Do not pass dragover to children
It seems to be more performant.

API-wise it makes sense that withDraggable takes care of calling
children's dragstart/dragend event handlers as it needs them
to be executed after its own. The reason for this is that the
children's event handlers could modify the DOM element
withDraggable clones, messing up with its behavior.
For example, if the children's event handler change the visibility of
the element that takes the event, in turn the element won't be able
to process any further events.

We don't have the same restrictions with the dragover event.
At that point, it can work independently from the children's.
Clarify BlockDraggable abstraction
It is the component that build the necessary transfer data
to drag a block around.

@nosolosw nosolosw force-pushed the add/with-draggable branch from 53b10d3 to 755e0aa Sep 3, 2018

@nosolosw nosolosw merged commit 4528ee3 into master Sep 3, 2018

2 checks passed

codecov/project 50.36% (-0.02%) compared to c9668e9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Sep 3, 2018

@youknowriad youknowriad deleted the add/with-draggable branch Sep 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.