-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from 23 commits
b435722
b5a35cd
18ef675
2d48f1a
26a0b6f
2ca2e4a
eb0c491
f545653
7610b9e
c9b44f2
07d1749
06ae3ba
d9a1807
25297ce
cc43d33
0427c19
f340eca
7829554
2d4c207
96c337c
7cd7f62
f568bf5
755e0aa
32006d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
||
|
@@ -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 | ||
|
@@ -49,10 +51,54 @@ const MyDraggable = () => ( | |
elementId="draggable-panel" | ||
transferData={ { } } | ||
> | ||
<Dashicon icon="move" /> | ||
{ | ||
( { onDraggableStart, onDraggableEnd } ) => ( | ||
<Dashicon | ||
icon="move" | ||
onDragStart={ onDraggableStart } | ||
onDragEnd={ onDraggableEnd } | ||
draggable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI @nosolosw: the See here: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/draggable cc @youknowriad There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dsifford you're right in that |
||
/> | ||
) | ||
} | ||
</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; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here: is there a better way to explain the change? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 } ) => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Maybe we should also pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess my comment assumes we rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That way the I like the current proposal better because it has a clearer "API contract": There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to explain this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deprecation will be removed in 4.0 release if it is planned for 3.8 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to include the version that will remove the behavior here? I haven't seen it anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be under 4.0 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✍️ 32006d3