-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Reorder blocks via drag & drop (v2. using editor dropzones). #4115
Changes from 66 commits
b62160a
7723d70
51cd0f5
82e9886
d6df73d
f38db58
1b04b5b
9f25b10
0a23609
9f3b3af
52ac1ff
c55ebc3
6c7a913
8dbc69e
35f9923
9271649
8fdcd6e
1e2ae5e
ec5052c
62afc6d
8f13212
883ab19
6faceef
4978348
b512271
e2c2e61
8436f21
f131740
2599d42
6ad1c44
6fc98de
178cc60
c98ea9c
6c2bf61
ab68b11
702eec3
7cbdfde
b0f0b0e
43ec29a
ec53f4f
2e0b13a
a2246f4
db7a34b
d8bedee
25485f9
05f2d10
f33d4db
a83c738
30ee308
da001c9
b071cc4
46ed7a2
129dac9
d6897ce
4e66a4a
bfb78c1
0a6a05d
f72e05d
f03e4ec
650f613
5c1d339
5c1613d
4d3b8b8
d731edf
9624dca
94dd1cc
7f01121
9982bcd
5623a71
f48d65a
d415a1f
0eca12c
e763e28
f19dcd0
48f8087
2759481
51c1cef
794dfd0
4a817c5
7dcc860
b20b033
c61c770
d5daf30
a648f67
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 |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# Draggable | ||
|
||
`Draggable` is a ReactComponent that can wrap any React 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. | ||
|
||
## Props | ||
|
||
The component accepts the following props: | ||
|
||
### elementId | ||
|
||
The HTML id of the element to clone on drag | ||
|
||
- Type: `string` | ||
- Required: Yes | ||
|
||
### transferData | ||
|
||
The data object attached to the drag and drop event. | ||
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. What is the shape of the object? Is it arbitrary? If so, should we note that it's arbitrary? |
||
|
||
- Type: `Object` | ||
- Required: Yes | ||
|
||
### onDragStart | ||
|
||
The function called when dragging starts. | ||
|
||
- Type: `Function` | ||
- Required: No | ||
- Default: `noop` | ||
|
||
### onDragEnd | ||
|
||
The function called when dragging ends. | ||
|
||
- Type: `Function` | ||
- Required: No | ||
- Default: `noop` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { noop } from 'lodash'; | ||
import classnames from 'classnames'; | ||
|
||
/** | ||
* WordPress Dependencies | ||
*/ | ||
import { Component } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal Dependencies | ||
*/ | ||
import withSafeTimeout from '../higher-order/with-safe-timeout'; | ||
import './style.scss'; | ||
|
||
const dragImageClass = 'components-draggable__invisible-drag-image'; | ||
const cloneWrapperClass = 'components-draggable__clone'; | ||
const cloneHeightTransformationBreakpoint = 700; | ||
const clonePadding = 20; | ||
|
||
class Draggable extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
this.onDragStart = this.onDragStart.bind( this ); | ||
this.onDragOver = this.onDragOver.bind( this ); | ||
this.onDragEnd = this.onDragEnd.bind( this ); | ||
} | ||
|
||
/** | ||
* Removes the element clone, resets cursor, and removes drag listener. | ||
* @param {Object} event The non-custom DragEvent. | ||
*/ | ||
onDragEnd( event ) { | ||
const { onDragEnd = noop } = this.props; | ||
this.removeDragClone(); | ||
// Reset cursor. | ||
document.body.classList.remove( 'dragging' ); | ||
event.preventDefault(); | ||
|
||
this.props.setTimeout( onDragEnd ); | ||
} | ||
|
||
/* | ||
* Updates positioning of element clone based on mouse movement during dragging. | ||
* @param {Object} event The non-custom DragEvent. | ||
*/ | ||
onDragOver( event ) { | ||
this.cloneWrapper.style.top = | ||
`${ parseInt( this.cloneWrapper.style.top, 10 ) + event.clientY - this.cursorTop }px`; | ||
this.cloneWrapper.style.left = | ||
`${ parseInt( this.cloneWrapper.style.left, 10 ) + event.clientX - this.cursorLeft }px`; | ||
|
||
// Update cursor coordinates. | ||
this.cursorLeft = event.clientX; | ||
this.cursorTop = event.clientY; | ||
} | ||
|
||
/** | ||
* - Clones the current element and spawns clone over original element. | ||
* - Adds a fake temporary drag image to avoid browser defaults. | ||
* - Sets transfer data. | ||
* - Adds dragover listener. | ||
* @param {Object} event The non-custom DragEvent. | ||
* @param {string} elementId The HTML id of the element to be dragged. | ||
* @param {Object} transferData The data to be set to the event's dataTransfer - to be accessible in any later drop logic. | ||
*/ | ||
onDragStart( event ) { | ||
const { elementId, transferData, onDragStart = noop } = this.props; | ||
const element = document.getElementById( elementId ); | ||
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. Normally, in React, we'd use If we stick to passing |
||
const elementWrapper = element.parentNode; | ||
const elementRect = element.getBoundingClientRect(); | ||
const elementTopOffset = parseInt( elementRect.top, 10 ); | ||
const elementLeftOffset = parseInt( elementRect.left, 10 ); | ||
const clone = element.cloneNode( true ); | ||
const dragImage = document.createElement( 'div' ); | ||
|
||
// Set a fake drag image to avoid browser defaults. Remove from DOM right after. | ||
if ( 'function' === typeof event.dataTransfer.setDragImage ) { | ||
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. Why do we check ...And as a follow-up, should we note via code comment that it's for browser compatibility? 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. Of course - will do. |
||
dragImage.id = `drag-image-${ elementId }`; | ||
dragImage.classList.add( dragImageClass ); | ||
document.body.appendChild( dragImage ); | ||
event.dataTransfer.setDragImage( dragImage, 0, 0 ); | ||
this.props.setTimeout( ( ( _dragImage ) => () => { | ||
document.body.removeChild( _dragImage ); | ||
} )( dragImage ), 0 ); | ||
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. Why the IIFE? Shouldn't 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.
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's not true though. The outer scope would not be shared between the two event callbacks. In demonstration form: https://codepen.io/aduth/pen/EEvKLM https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures 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. You are of course absolutely right. That would only make sense if 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.
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout 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. Of course. I had a comment by someone else at some point earlier to include the default, but I'd prefer to not have it also. 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 one may argue it's good to be explicit, but beyond being noise, it's also good to know the default behavior of functions. |
||
} | ||
|
||
event.dataTransfer.setData( 'text', JSON.stringify( transferData ) ); | ||
|
||
// Prepare element clone and append to element wrapper. | ||
clone.id = `clone-${ elementId }`; | ||
this.cloneWrapper = document.createElement( 'div' ); | ||
this.cloneWrapper.classList.add( cloneWrapperClass ); | ||
this.cloneWrapper.style.width = `${ elementRect.width + ( clonePadding * 2 ) }px`; | ||
|
||
if ( elementRect.height > cloneHeightTransformationBreakpoint ) { | ||
// Scale down clone if original element is larger than 700px. | ||
this.cloneWrapper.style.transform = 'scale(0.5)'; | ||
this.cloneWrapper.style.transformOrigin = 'top left'; | ||
// Position clone near the cursor. | ||
this.cloneWrapper.style.top = `${ event.clientY - 100 }px`; | ||
this.cloneWrapper.style.left = `${ event.clientX }px`; | ||
} else { | ||
// Position clone right over the original element (20px padding). | ||
this.cloneWrapper.style.top = `${ elementTopOffset - clonePadding }px`; | ||
this.cloneWrapper.style.left = `${ elementLeftOffset - clonePadding }px`; | ||
} | ||
|
||
this.cloneWrapper.appendChild( clone ); | ||
elementWrapper.appendChild( this.cloneWrapper ); | ||
|
||
// Mark the current cursor coordinates. | ||
this.cursorLeft = event.clientX; | ||
this.cursorTop = event.clientY; | ||
// Update cursor to 'grabbing', document wide. | ||
document.body.classList.add( 'dragging' ); | ||
document.addEventListener( 'dragover', this.onDragOver ); | ||
|
||
this.props.setTimeout( onDragStart ); | ||
} | ||
|
||
componentWillUnmount() { | ||
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: Prefer lifecycle defined together at top of component's methods. |
||
this.removeDragClone(); | ||
} | ||
|
||
removeDragClone() { | ||
document.removeEventListener( 'dragover', this.onDragOver ); | ||
if ( this.cloneWrapper && this.cloneWrapper.parentElement ) { | ||
// Remove clone. | ||
this.cloneWrapper.remove(); | ||
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. It doesn't look like Elsewhere in the project, |
||
this.cloneWrapper = null; | ||
} | ||
} | ||
|
||
render() { | ||
const { children, className } = this.props; | ||
return ( | ||
<div | ||
className={ classnames( 'components-draggable', className ) } | ||
onDragStart={ this.onDragStart } | ||
onDragEnd={ this.onDragEnd } | ||
draggable | ||
> | ||
{ children } | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
export default withSafeTimeout( Draggable ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
body.dragging { | ||
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. Is a 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. Definitely not - need to think of a better name here. 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. Updated the className to use |
||
cursor: move;/* Fallback for IE/Edge < 14 */ | ||
cursor: grabbing !important; | ||
} | ||
|
||
.components-draggable__invisible-drag-image { | ||
position: fixed; | ||
left: -1000px; | ||
height: 50px; | ||
width: 50px; | ||
} | ||
|
||
.components-draggable__clone { | ||
position: fixed; | ||
padding: 20px; | ||
background: transparent; | ||
pointer-events: none; | ||
z-index: z-index( '.components-draggable__clone' ); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,17 @@ $z-layers: ( | |
'.edit-post-header': 30, | ||
'.wp-block-image__resize-handlers': 1, // Resize handlers above sibling inserter | ||
|
||
// Should have lower index than anything else positioned inside the block containers | ||
'.editor-block-list__block-drag-inset': 0, | ||
// Should have higher index than anything else positioned inside the block containers | ||
'.editor-block-list__block-drag-inset.is-visible > .inner': 1000000000, | ||
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. These numbers are becoming increasingly... increasing. Why does this need to be 1000x higher than the next-highest z-index? The issue with z-index is we always think our thing ought to be higher than all the things, but when all elements act this way, it's a never-ending battle. 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. Agreed - one of those things that need to be cleaned up. I just bumped up those values to make sure they are working as expected. Not sure though what the best way to ensure they "have higher index than anything else positioned inside the block containers". |
||
|
||
'.components-draggable__clone': 1000000000, | ||
|
||
// Should have higher index than the inset/underlay used for dragging | ||
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. Why? 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 believe the drag inset was blocking some controls/buttons used for uploading images. Can you try this with a lower index than 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. Ok so I tried removing this entirely and I can't see any issue at the moment. 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 it's needed :) for the gray background when dragging 🤕 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. The z-index itself is not really needed. 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. Could be. The issue wasn't related with the "clone", but with the actual panel that shows up when you add an image block but want to click to upload instead of drag an image from the desktop. Not sure if this was removed too. Let me load up Gutenberg to confirm then. 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.
Conversely, we should be cautious with adding them as well, and as made obvious by this thread, very clearly document their intention. 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 can click the buttons (Upload and Add media from Library) properly. Was this the issue? 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.
Absolutely. I didn't mean for my answer to excuse my clumsiness in adding these - I just didn't know any better. :-) 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.
Ok. So the relation is still there. If I set |
||
'.components-placeholder__fieldset': 1, | ||
'.editor-block-list__block-edit .reusable-block-edit-panel *': 1, | ||
|
||
// Show drop zone above most standard content, but below any overlays | ||
'.components-drop-zone': 100, | ||
'.components-drop-zone__content': 110, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ import { compose } from '@wordpress/element'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { insertBlocks, updateBlockAttributes } from '../../store/actions'; | ||
import { insertBlocks, updateBlockAttributes, moveBlockToPosition } from '../../store/actions'; | ||
|
||
function BlockDropZone( { index, isLocked, ...props } ) { | ||
if ( isLocked ) { | ||
|
@@ -53,10 +53,32 @@ function BlockDropZone( { index, isLocked, ...props } ) { | |
} | ||
}; | ||
|
||
const onDrop = ( event, position ) => { | ||
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. For a component which is rendered once-per-block, I wish we were more conscious about the performance implications of creating new function references on every render. (Not entirely related to this pull request, since the pattern has already been established here). |
||
if ( ! event.dataTransfer ) { | ||
return; | ||
} | ||
|
||
let uid, type, rootUID, fromIndex; | ||
|
||
try { | ||
( { uid, type, rootUID, fromIndex } = JSON.parse( event.dataTransfer.getData( 'text' ) ) ); | ||
} catch ( err ) { | ||
return; | ||
} | ||
|
||
if ( type !== 'block' ) { | ||
return; | ||
} | ||
const positionIndex = getInsertIndex( position ); | ||
const insertIndex = index && fromIndex < index && rootUID === props.rootUID ? positionIndex - 1 : positionIndex; | ||
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. What's being reflected in the logic here? Please add a code comment. |
||
props.moveBlockToPosition( uid, rootUID, insertIndex ); | ||
}; | ||
|
||
return ( | ||
<DropZone | ||
onFilesDrop={ onDropFiles } | ||
onHTMLDrop={ onHTMLDrop } | ||
onDrop={ onDrop } | ||
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. Naming doesn't seem consistent between the two properties – 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. Indeed. I could 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. Unrelated: When does 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 think it's the former (called on every drop), but need to double check. |
||
/> | ||
); | ||
} | ||
|
@@ -84,8 +106,12 @@ export default compose( | |
updateBlockAttributes( ...args ) { | ||
dispatch( updateBlockAttributes( ...args ) ); | ||
}, | ||
moveBlockToPosition( uid, fromRootUID, index ) { | ||
const { rootUID, layout } = ownProps; | ||
dispatch( moveBlockToPosition( uid, fromRootUID, rootUID, layout, index ) ); | ||
}, | ||
}; | ||
} | ||
}, | ||
), | ||
withContext( 'editor' )( ( settings ) => { | ||
const { templateLock } = settings; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import classnames from 'classnames'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { Draggable } from '@wordpress/components'; | ||
|
||
function BlockDraggable( { rootUID, index, uid, layout, isDragging, ...props } ) { | ||
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: This component shouldn't need to rely on the parent to pass in 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's true, but it's needed also on the parent so I thought about avoiding the duplicate logic. Also, originally this component could have been used several times for the same block, which means 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. Fair enough. If it was a separate component not within the same folder, I think it'd be a bigger issue, but for these types of components I think it's okay-ish to make some assumptions about the behavior of the parent component. |
||
const blockDragInsetClassName = classnames( 'editor-block-list__block-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. Does the variable name need to be so intricate? The name itself is not very clear to me in what context it's providing. 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'm guessing just 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. Or just 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. Ah yes, this is no longer in |
||
'is-visible': isDragging, | ||
} ); | ||
|
||
const transferData = { | ||
type: 'block', | ||
rootUID: rootUID, | ||
fromIndex: index, | ||
uid: uid, | ||
layout, | ||
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: Inconsistency: We use object property shorthand for some, but not all properties. |
||
}; | ||
|
||
return ( | ||
<Draggable className={ blockDragInsetClassName } transferData={ transferData } { ...props }> | ||
<div className="inner" ></div> | ||
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. The class name does not adhere to standards, and is extremely susceptible to conflicts. 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. If we're expecting extending 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. Space before opening 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. Not certain I understand the purpose of a 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. What's the purpose of |
||
</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.
I'm tending to avoid references to "React" directly. With our abstraction layer in place, it can muddy the distinction (are these components compatible). Rather, I find it tends to be fine to avoid qualifying it altogether, just labeling it a "component" or "element".