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