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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b435722
Add withDraggable HOC
oandregal Aug 24, 2018
b5a35cd
Deprecate Draggable
oandregal Aug 24, 2018
18ef675
Convert block-draggable into class
oandregal Aug 27, 2018
2d48f1a
Inline Draggable component
oandregal Aug 27, 2018
26a0b6f
Use withDraggable
oandregal Aug 27, 2018
2ca2e4a
Pass props to wrapped component
oandregal Aug 29, 2018
eb0c491
withDraggable is responsible to call the props passed to the wrapped …
oandregal Aug 29, 2018
f545653
BlockDraggable: adapt to new API
oandregal Aug 29, 2018
7610b9e
Fix dragend event handler
oandregal Aug 29, 2018
c9b44f2
withDraggable's event handlers take control
oandregal Aug 29, 2018
07d1749
Make setTimeout property available within withDraggable
oandregal Aug 29, 2018
06ae3ba
Do not pass dragover to children
oandregal Aug 29, 2018
d9a1807
Make Draggable ready to process children functions
oandregal Aug 30, 2018
25297ce
Remove any traces of withDraggable HOC
oandregal Aug 30, 2018
cc43d33
Move deprecation to correct version
oandregal Aug 30, 2018
0427c19
BlockDraggable: convert it back to a functional component
oandregal Aug 30, 2018
f340eca
Update docs
oandregal Aug 30, 2018
7829554
Update docs
oandregal Aug 30, 2018
2d4c207
Clarify BlockDraggable abstraction
oandregal Aug 30, 2018
96c337c
Update docs
oandregal Aug 30, 2018
7cd7f62
Update deprecation
oandregal Aug 30, 2018
f568bf5
Add note in CHANGELOG
oandregal Aug 30, 2018
755e0aa
Update version that will depracate the behavior
oandregal Aug 30, 2018
32006d3
Add deprecation within 4.0 section
oandregal Sep 3, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/reference/deprecated.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
## 4.0.0

- `wp.components.RichTextProvider` has been removed. Please use `wp.data.select( 'core/editor' )` methods instead.
- `wp.components.Draggable` as a DOM node drag handler has been removed. Please, use `wp.components.Draggable` as a wrap component for your DOM node drag handler.

## 3.9.0

Expand Down
3 changes: 2 additions & 1 deletion packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@

### Breaking Change

- `withAPIData` has been removed. Please use the Core Data module or `@wordpress/api-fetch` directly instead.
- `withAPIData` has been removed. Please use the Core Data module or `@wordpress/api-fetch` directly 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.
54 changes: 50 additions & 4 deletions packages/components/src/draggable/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Draggable

`Draggable` is a Component that can wrap any element to make it draggable. When used, a cross-browser (including IE) customisable drag image is created. The component clones the specified element on drag-start and uses the clone as a drag image during drag-over. Discards the clone on drag-end.
`Draggable` is a Component that provides a way to set up a a cross-browser (including IE) customisable drag image and the transfer data for the drag event. It decouples the drag handle and the element to drag: use it by wrapping the component that will become the drag handle and providing the DOM ID of the element to drag.

Note that the drag handle needs to declare the `draggable="true"` property and bind the `Draggable`s `onDraggableStart` and `onDraggableEnd` event handlers to its own `onDragStart` and `onDragEnd` respectively. `Draggable` takes care of the logic to setup the drag image and the transfer data, but is not concerned with creating an actual DOM element that is draggable.

## Props

Expand All @@ -22,15 +24,15 @@ Arbitrary data object attached to the drag and drop event.

### onDragStart

The function called when dragging starts.
A function to be called when dragging starts.

- Type: `Function`
- Required: No
- Default: `noop`

### onDragEnd

The function called when dragging ends.
A function to be called when dragging ends.

- Type: `Function`
- Required: No
Expand All @@ -49,10 +51,54 @@ const MyDraggable = () => (
elementId="draggable-panel"
transferData={ { } }
>
<Dashicon icon="move" />
{
( { onDraggableStart, onDraggableEnd } ) => (
<Dashicon
icon="move"
onDragStart={ onDraggableStart }
onDragEnd={ onDraggableEnd }
draggable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/>
)
}
</Draggable>
</PanelBody>
</Panel>
</div>
);
export default MyDraggable;
```

In case you want to call your own `dragstart` / `dragend` event handlers as well, you can pass them to `Draggable` and it'll take care of calling them after their own:

```jsx
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;
```
14 changes: 14 additions & 0 deletions packages/components/src/draggable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import classnames from 'classnames';
*/
import { Component } from '@wordpress/element';
import { withSafeTimeout } from '@wordpress/compose';
import deprecated from '@wordpress/deprecated';

const dragImageClass = 'components-draggable__invisible-drag-image';
const cloneWrapperClass = 'components-draggable__clone';
Expand Down Expand Up @@ -148,6 +149,19 @@ class Draggable extends Component {

render() {
const { children, className } = this.props;
if ( typeof children === 'function' ) {
return children( {
onDraggableStart: this.onDragStart,
onDraggableEnd: this.onDragEnd,
} );
}

deprecated( 'wp.components.Draggable as a DOM node drag handle', {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: is there a better way to explain the change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm wondering if we should really deprecate this or just consider it as a fallback in case there's no children as function prop provided

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a specific use case where that'd be useful, I'd rather deprecate it. Less code to maintain and understand.

version: 4.0,
alternative: 'wp.components.Draggable as a wrapper component for a DOM node',
plugin: 'Gutenberg',
} );

return (
<div
className={ classnames( 'components-draggable', className ) }
Expand Down
25 changes: 20 additions & 5 deletions packages/editor/src/components/block-list/block-draggable.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,39 @@ import classnames from 'classnames';
*/
import { Draggable } from '@wordpress/components';

function BlockDraggable( { rootClientId, index, clientId, layout, isDragging, ...props } ) {
const BlockDraggable = ( { clientId, rootClientId, blockElementId, layout, order, isDragging, onDragStart, onDragEnd } ) => {
const className = classnames( 'editor-block-list__block-draggable', {
'is-visible': isDragging,
} );

const transferData = {
type: 'block',
fromIndex: index,
fromIndex: order,
rootClientId,
clientId,
layout,
};

return (
<Draggable className={ className } transferData={ transferData } { ...props }>
<div className="editor-block-list__block-draggable-inner"></div>
<Draggable
elementId={ blockElementId }
transferData={ transferData }
onDragStart={ onDragStart }
onDragEnd={ onDragEnd }
>
{
( { onDraggableStart, onDraggableEnd } ) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Maybe we should also pass the draggable prop here. That way we can just do { ...props } in the inner div. The idea is: The Draggable component provides all the necessary props to make dragging work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my comment assumes we rename onDraggableStart and onDraggableEnd into onDragStart and onDragEnd

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way the Draggable's onDragStart/onDragEnd properties would shadow the BlockDraggable's onDragStart / onDragEnd props. I know we can avoid that by using the ...props trick but it feels more obscure to me, like a smell of something that's not right.

I like the current proposal better because it has a clearer "API contract": Draggable provides event handlers for you to connect in a specific DOM node. That's it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Noting that it's a common pattern though in the React world (see https://github.com/paypal/downshift)

<div
className={ className }
onDragStart={ onDraggableStart }
onDragEnd={ onDraggableEnd }
draggable
>
<div className="editor-block-list__block-draggable-inner"></div>
</div> )
}
</Draggable>
);
}
};

export default BlockDraggable;
6 changes: 3 additions & 3 deletions packages/editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -483,14 +483,14 @@ export class BlockListBlock extends Component {
>
{ ! isPartOfMultiSelection && isMovable && (
<BlockDraggable
rootClientId={ rootClientId }
index={ order }
clientId={ clientId }
rootClientId={ rootClientId }
blockElementId={ blockElementId }
layout={ layout }
order={ order }
onDragStart={ this.onDragStart }
onDragEnd={ this.onDragEnd }
isDragging={ dragging }
elementId={ blockElementId }
/>
) }
{ shouldShowInsertionPoint && (
Expand Down