Reorder blocks via drag & drop (v2. using editor dropzones).#4115
Conversation
- Updated reducers and actions to allow reindexing of blocks given uid of block and new index position. - Added respective tests.
… and reordering blocks. - Using the current dropzones and onDrop handlers. - Added background overlay to blocks for using as drag area. - Added drag handling to block mover so we can grab the block from there. - Added global styling for changing the cursor to hand.
|
You posted a screencast in Slack that's looking really good. Very nice work! Some questions:
A few wishlist items for the implementation of block drag and drop:
A bunch of us are going on holiday and won't return until early in the new year, so I hope you can be patient about reviews until then ❤️ — thanks again for working on this. |
|
Hey Joen! Thank you very much for your detailed response. This is exactly the type of feedback I wanted at this point 😃 Now I know what to focus on.
The purple area is what responds to the drag event, but it currently sits in the background so that it doesn't take precedence over other drag events (i.e. resizing images). I haven't really put any thought on making the whole block draggable, so this was just a quick assembly. We can definitely investigate tweaking z-indexes and adding flags where necessary to give a broader drag handle.
I haven't tried yet, but definitely aim for this to support the browsers in our list or fail gracefully.
It's in my list of things to do/investigate next. So we will have (hopefully) some sort of scrolling, although we may need something more than scrolling for the 20 pages case. Imagine you scroll 19 pages and drop the handle... that would not be so nice 😁
Indeed. I do plan to investigate touch handling, so maybe once we are pixel perfect for desktop we can discuss how to incorporate this for mobile. I also need to work on desktop narrow view, as I have only fiddled with a wide viewport right now.
No problem! I just used F8F8F8 in the prototype.
Shouldn't be a problem to fix. I can add an inner underlay and only show that when dragging - and we can adjust its size as we like.
Yes, although I haven't had time to look deeper into this, so it may not be a big issue eventually. The problem that I seem to had was as follows: We take a snapshot of the DOM element (block) and that is what goes in the preview. So whatever we use for preview has to be visible at the time. Adding the shadow created a sort of flickering effect (in that we add the shadow, take the snapshot, and then hide the element). It didn't feel natural, so I may need to find an alternative solution. Might also be that I do not clone the DOM element currently - I just use it for the snapshot and then hide it. I have read that others clone the element, place it somewhere visible on the page (visible to the browser, not the user - so effectively outside the viewport). So this is something that I need to investigate. I do not know the solution or the sort of problems that may arise from cloning a DOM element - but will definitely try a few things out. We also have full opacity in the mocks, but we get a transparent image in the prototype, so there's that too 😃
Nit a problem. Will look into this.
Not a problem at all. I believe I have enough feedback now to carry on 😃 Once we are happy with it visually, then we can work out the best solution for a more reusable composition. |
|
Thanks again for the wonderful feedback Joen! ❤️ I will keep you posted on progress, share a few more screencasts, etc. as I carry on. 😎 |
|
Awesome!
Sometimes when I need to animate something, like a shadow appearing, I apply an invisible box shadow to the element, always there, like Might that work? In any case, not critical, but something to think about. |
Nice! I think that's a great idea. I will experiment with it next. 😎 |
…e drop position. Updated the precision of determining the drop position for a block. Previously, this was borrowed from the "drop files" logic. This created inconsistencies due to not accounting that a block is being removed from its position and reindexed.
- Added inner div to underlay container. - Inner div's area can be adjusted further to desirable size.
block. - Set to grab when on hover. - Set to grabbing when dragging. - Set the same hover rule for the block mover (the original takes precedence when hovering over the arrows).
…nt in order to prioritise over the inset/underlay used for dragging.
… on larger views only.
…st master changes. - An error surfaced that caused both onDrop and onFilesDrop handlers to fire when dragging a block over a drop zone. - Fixed it by conditioning on the "DataTransfer.effectAllowed" property. - Set "move" to be the effect used for reordering elements and everything else for dropping files.
…set', and wrapped in a timeout the resetting of the block display after a drag.
- Added a 'type' flag in the event data transfer. - Potentially this approach will facilitate multiple onDrop handlers for reordering other areas (i.e. images in a gallery - we'd use a different constant in that case).
DataTransfer.setDragImage. - IE11 does not support this call. - Dragging/reordering continues to work as expected in IE11 but without the custom drag image. - The gray inset area is also set as expected in IE11.
- Updated conditioning for the files drop logic to be on availability of
files in the files list.
- Neither of the above browsers support the drop effect api for drag &
drop.
- Need a way to distinguish between a file upload and a different drop
effect.
- Added a clone node used for customising and setting the drag image. - Includes a shadow in Chrome and Firefox - no shadow in Safari. - No drag image for IE11 yet. Same logic however can be extended to drag around the clone instead. - Updated the location of showing the drag image - it will now appear right over the block to be reordered (more natural this way).
Chrome, Firefox, and Safari. - We create a block clone and spawn it right over the block. - Set is as drag image and remove it from the DOM right after. - Behaviour is consistent now with all of the above browsers.
|
Hi friends, I'd like to merge this soon to have plenty of time to detect any regression before the next release. I'd appreciate a 👍 |
| "re-resizable": "4.0.3", | ||
| "react": "16.2.0", | ||
| "react-autosize-textarea": "2.0.0", | ||
| "react-autosize-textarea": "3.0.2", |
There was a problem hiding this comment.
Originally I thought the bug was specific to this PR,I can remove the commit from this PR.
| rootUID: rootUID, | ||
| fromIndex: index, | ||
| uid: uid, | ||
| layout, |
There was a problem hiding this comment.
Minor: Inconsistency: We use object property shorthand for some, but not all properties. uid and rootUID qualify as well.
|
|
||
| return ( | ||
| <Draggable className={ blockDragInsetClassName } transferData={ transferData } { ...props }> | ||
| <div className="inner" ></div> |
There was a problem hiding this comment.
What's the purpose of <div className="editor-block-list__block-draggable-inner"></div> ?
editor/store/actions.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Returns an aciton object signalling that an indexed block should be moved |
components/draggable/index.js
Outdated
|
|
||
| // Set a fake drag image to avoid browser defaults. Remove from DOM right after. | ||
| // event.dataTransfer.setDragImage is not supported yet in IE, | ||
| // we need to check for its existance first. |
There was a problem hiding this comment.
s/existance/existence. Bonus for line reformatting:
// Set a fake drag image to avoid browser defaults. Remove from DOM
// right after. event.dataTransfer.setDragImage is not supported yet in
// IE, we need to check for its existance first.
| */ | ||
| onDragStart( event ) { | ||
| const { elementId, transferData, onDragStart = noop } = this.props; | ||
| const element = document.getElementById( elementId ); |
There was a problem hiding this comment.
Normally, in React, we'd use ref for this sort of thing. I understand that BlockListBlock is a bit different in that BlockDraggable is nested in the element itself via IgnoreNestedEvents, thus making it trickier to grab and pass the right ref. Edit: Actually, passing the ID string may make this component more easily reusable 👍.
If we stick to passing elementId, which seems fine, should we make sure that element exists?
components/draggable/index.js
Outdated
| document.removeEventListener( 'dragover', this.onDragOver ); | ||
| if ( this.cloneWrapper && this.cloneWrapper.parentElement ) { | ||
| // Remove clone. | ||
| this.cloneWrapper.remove(); |
There was a problem hiding this comment.
It doesn't look like .remove is supported in IE11: https://caniuse.com/#feat=childnode-remove
Elsewhere in the project, .parentNode.removeChild is used.
| onDragStart={ this.onDragStart } | ||
| onDragEnd={ this.onDragEnd } | ||
| isDragging={ dragging } | ||
| elementId={ blockElementId } |
There was a problem hiding this comment.
The disconnect between this component and the element it clones is unclear to me. At a glance, it seems more sensible to me <Draggable> should make its child draggable, not target the target via a DOM ID.
There was a problem hiding this comment.
The elementId corresponds to the element to clone so it's not exactly the draggable element. Maybe we could rename the prop to clarify it?
| } | ||
|
|
||
| .editor-block-list__block-edit .reusable-block-edit-panel * { | ||
| z-index: z-index( '.editor-block-list__block-edit .reusable-block-edit-panel *' ); |
There was a problem hiding this comment.
What's this for? Does it need to be applied as wildcard to all its children? Or is the container enough?
There was a problem hiding this comment.
I've answered this above: https://github.com/WordPress/gutenberg/pull/4115/files/51c1cefc4081f4965d1db68fe7e6dd6f02ae7d8e#r178302159 Not sure why I decided to apply it on all its immediate children, but might be due to multiple controls being blocked in the panel.
There was a problem hiding this comment.
I'd like to see if we can simplify this to eliminate the wildcard selector.
| top: 0; | ||
| padding: 0; | ||
|
|
||
| /* Necessary for drag indicator */ |
There was a problem hiding this comment.
It's for showing the grab handle when hovering over the area.
| .editor-block-list .editor-inserter { | ||
| margin: $item-spacing; | ||
| cursor: move;/* Fallback for IE/Edge < 14 */ | ||
| cursor: grab; |
There was a problem hiding this comment.
It's for showing the grab handle when hovering over the area - probably the area covered by the inserter container in this case - the actual arrows I believe apply a different handle.
There was a problem hiding this comment.
At the very least, sounds like a good inline code comment.
At best, we ought not need to sprinkle these all around the stylesheets.
There was a problem hiding this comment.
At best, we ought not need to sprinkle these all around the stylesheets.
That is correct. So we would need some type of logic to apply the styling when we hover at the different locations - cause there are different stacks if I recall (the ellipsis menu has an area around it and sits above the drag inset. So does the inserter menu I believe). Maybe the approach of having an underlay sitting in the background wasn't ideal after all.
| const { index } = this.props; | ||
| const positionIndex = this.getInsertIndex( position ); | ||
| // If the block is kept at the same level and moved downwards, | ||
| // we need to substract "1" from the insert index. |
There was a problem hiding this comment.
My first though was "but why?" After thinking some more, I started to understand it's to account for the fact that the block is no longer where it was, so all other blocks below it shift upward. Could be more clear.
There was a problem hiding this comment.
I don't know how to express this honestly :) Some help would be appreciated.
There was a problem hiding this comment.
I had a hard time explaining what happens when you remove an item from a list and insert it in a different position. The indexes change. Maybe my response here can shed some light: #4115 (comment) (about creating a general function to move 1+ blocks to a different index). If not, give me a ping to try again.
There was a problem hiding this comment.
Maybe:
// If the block is kept at the same level and moved downwards, subtract
// to account for blocks shifting upward to occupy its old position.| right: 0; | ||
| bottom: 0; | ||
| left: 0; | ||
| z-index: z-index( '.editor-block-list__block-drag-inset' ); |
There was a problem hiding this comment.
Why do we need this? What happens if it's not here?
There was a problem hiding this comment.
We want the drag inset to have a lower initial index than anything else positioned absolutely within the block to not interfere or be visible in any way. Then we want the drag inset to have a higher index than everything else to cover the area with the grey background when the block clone is being dragged.
There was a problem hiding this comment.
We want the drag inset to have a lower initial index than anything else positioned absolutely within the block to not interfere or be visible in any way.
Is that not already the case if it's absolutely positioned and the first child element?
Then we want the drag inset to have a higher index than everything else to cover the area with the grey background when the block clone is being dragged.
Isn't this already achieved by .is-hidden applying visibility: none?
There was a problem hiding this comment.
Is that not already the case if it's absolutely positioned and the first child element?
If we can guarantee that it will always be the first child element.
Isn't this already achieved by .is-hidden applying visibility: none?
That covers everything inside editor-block-list__block. If that's enough, then yes.
| 'is-multi-selected': isMultiSelected, | ||
| 'is-hovered': isHovered, | ||
| 'is-reusable': isReusableBlock( blockType ), | ||
| 'is-hidden': dragging, |
There was a problem hiding this comment.
Is this a concern of the consumer of Draggable ? Seems to require a lot of work to use.
There was a problem hiding this comment.
That is correct - showing an overlay in place of the original content or some other style deviation is the concern of the consumer.
|
|
||
| '.components-draggable__clone': 1000000000, | ||
|
|
||
| // Should have higher index than the inset/underlay used for dragging |
There was a problem hiding this comment.
I believe the drag inset was blocking some controls/buttons used for uploading images. Can you try this with a lower index than '.editor-block-list__block-drag-inset' above to confirm?
There was a problem hiding this comment.
Ok so I tried removing this entirely and I can't see any issue at the moment.
There was a problem hiding this comment.
Actually it's needed :) for the gray background when dragging 🤕
There was a problem hiding this comment.
The z-index itself is not really needed.
There was a problem hiding this comment.
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.
Yeah, i'd be a bit cautious removing these indexes
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.
I can click the buttons (Upload and Add media from Library) properly. Was this the issue?
There was a problem hiding this comment.
Conversely, we should be cautious with adding them as well, and as made obvious by this thread, very clearly document their intention.
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.
I can click the buttons (Upload and Add media from Library) properly. Was this the issue?
Ok. So the relation is still there. If I set '.editor-block-list__block-draggable' to a higher index than the other two, I cannot click or interact with the controls. I just confirmed with the latest version. But you probably right in saying that "The z-index itself is not really needed." since it defaults to 0. Not sure why I had to be explicit about these.
|
|
||
| return ( | ||
| <DropZone | ||
| onFilesDrop={ this.onDropFiles } |
There was a problem hiding this comment.
onDrop |
onDrop |
onHTMLDrop |
onHTMLDrop |
onFilesDrop |
... onDropFiles ? |
components/draggable/index.js
Outdated
| this.props.setTimeout( onDragStart ); | ||
| } | ||
|
|
||
| componentWillUnmount() { |
There was a problem hiding this comment.
Minor: Prefer lifecycle defined together at top of component's methods.
8b939a4 to
7dcc860
Compare
2c7106c to
b20b033
Compare
|
@youknowriad I notice a minor inconsistency with the latest branch. Although the grab handle is shown when hovering over the inserter and ellipsis menu, I can’t seem to grab the block from there. We may as well not show it then if that functionality's been removed. (sorry that i just noticed this - browser: chrome) p.s. there was a bit of discussion earlier with Nikolay and Joen about enabling grabbing from there, and it was eventually consistent in all browsers in terms of functionality. |
|
This one needs a final rebase. Also, can we make it so that the new |
…-blocks--dropzone
|
Rebased, I tried adding a Draggable handler around |
aduth
left a comment
There was a problem hiding this comment.
Quibbling at this point. I'd like to see this merged. I think it could still use some continued polish, though we can do so iteratively. Particularly around: impulsive additions of z-index, cursor styles, burden of consuming component in integrating Draggable.
In final testing, I also found I'm still seeing the "grab" cursor become permanently stuck, though I have not been able to find consistent steps for reproducing.
| } | ||
|
|
||
| .editor-block-list__block-edit .reusable-block-edit-panel * { | ||
| z-index: z-index( '.editor-block-list__block-edit .reusable-block-edit-panel *' ); |
There was a problem hiding this comment.
I'd like to see if we can simplify this to eliminate the wildcard selector.
| const { index } = this.props; | ||
| const positionIndex = this.getInsertIndex( position ); | ||
| // If the block is kept at the same level and moved downwards, | ||
| // we need to substract "1" from the insert index. |
There was a problem hiding this comment.
Maybe:
// If the block is kept at the same level and moved downwards, subtract
// to account for blocks shifting upward to occupy its old position.
components/drop-zone/README.md
Outdated
|
|
||
| `DropZone` is a Component creating a drop zone area taking the full size of its parent element. It supports dropping files, HTML content or any other HTML drop event. To work properly this components needs to be wrapped in a `DropZoneProvider`. | ||
|
|
||
| ## Usage |
| function MyComponent() { | ||
| return ( | ||
| <DropZoneProvider> | ||
| <div> |
There was a problem hiding this comment.
Is the intermediate div necessary here?
aduth
left a comment
There was a problem hiding this comment.
Sorry, one more blocker: I see horizontal scroll on the demo that I don't see in master. Seems to be specifically caused by the full-align image in the "Media Rich" section of text.
|
Merged, there are still some improvements to make, but it should be easier to do as follow-ups. Thanks all for your work on this PR. Props to @chriskmnds |
|
HOOOLY GUACAMOOOOLEEEEE 🔥 🔥 🔥 |
| window.addEventListener( 'dragover', this.dragOverListener ); | ||
| window.addEventListener( 'drop', this.onDrop ); | ||
| window.addEventListener( 'mouseup', this.resetDragState ); | ||
| this.container = findDOMNode( this ); |
There was a problem hiding this comment.
We should have suppressed this warning with a valid reason for disabling.
Aside: IMO we shouldn't have ESLint warnings; only errors.

Description
This PR aims to introduce drag & drop functionality for rearranging blocks, and potentially for reordering other elements elsewhere in the editor (e.g. image galleries). Relevant issues: #38, #743, #1511.
I am continually updating this description with new issues, screenshots, etc. Last update: 10 March, 2018.
How Has This Been Tested?
Screenshots (jpeg or gifs if applicable):
The screenshots below will be updated accordingly as design/method changes are applied.
Dragging a block from the sides (this is an image from a previous version that includes some transparency in the resulting drag image - the current version doesn't):
Long blocks with height greater than
700pxget transformed (scaled down50%) when cloned:The underlay div mentioned below:
The inner underlay that depicts the actual gray area shown while dragging:
Types of Changes
This PR does not introduce external dependencies and instead attempts to make due with the current dropzones.
Most changes involve adding drag handlers to the block components. A Higher Order Component (HOC) has been added that provides drag initialisation/end handlers ( onDragStart and onDragEnd properties ) to the wrapped component (in this case, the block-list layout). DragOver is handled by the HOC and need not be fiddled with in the wrapped component. Drop handlers and styling are handled by the wrapped component. See the HOC's README file for further details. An action listener and reducer have been added to the store for reindexing a block when dropped onto one of the existing drop zones.
An underlay div (along an invisible inner div with grey background) was added to the block component and sits behind the rest of the controls. This essentially wraps the block area and responds to drag evens (see third image above for a more visual depiction). The same drag handlers have been added to the block-mover (up/down arrows) and block settings (ellipsis) components, so that users can grab the block from the mover controls as well.
There are 4 steps to the reordering process:
Browser Compatibility
Currently tested with latest:
Unless otherwise stated in the issues below, functionality is consistent across these browsers.
Checklist:
Known Issues:
List of issues to address/investigate. Some of these address @jasmussen's and @nb's comments below.
grabwhile hovering andgrabbingwhile dragging. Investigate IE/Edge fallback/polyfill.setDragImagesupport. Invent a solution that does not deviate too much from the rest. Keep in mind the bug/issue withonDragin Safari here: https://bugzilla.mozilla.org/show_bug.cgi?id=505521...additionalPropsblanket in places that it was used.masterchanges and the sibling inserter tweaks. Reconfirm design with @jasmussen. (Haven't noticed any visual regression or style changes, so marking this complete.)ellipsisandblock moverswithout passing the drag handlers to them.BLOCK_REORDERconstant set in data transfer during drag operation.Non-Blocking, Candidate Enhancements:
Optimise sensitivity of drop handling and dropzone indicators. Currently the cursor needs to be over the indicator for dropping to take effect.(overreach)