-
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
Reorder blocks via drag & drop (v2. using editor dropzones). #4115
Changes from 28 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 |
---|---|---|
|
@@ -15,6 +15,7 @@ import { compose } from '@wordpress/element'; | |
* Internal dependencies | ||
*/ | ||
import { insertBlocks } from '../../store/actions'; | ||
import { BLOCK_REORDER } from '../../store/constants'; | ||
|
||
function BlockDropZone( { index, isLocked, ...props } ) { | ||
if ( isLocked ) { | ||
|
@@ -43,9 +44,32 @@ function BlockDropZone( { index, isLocked, ...props } ) { | |
} | ||
}; | ||
|
||
const reorderBlock = ( event, position ) => { | ||
if ( index !== undefined && event.dataTransfer ) { | ||
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. We can avoid unnecessarily tabbing the rest of this function (harms readability) by inversing this condition and turning it into an early return: if ( index === undefined || ! event.dataTransfer ) {
return;
} 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, much cleaner - will refactor this. :-) |
||
try { | ||
const { uid, fromIndex, type } = JSON.parse( event.dataTransfer.getData( 'text' ) ); | ||
|
||
if ( type !== BLOCK_REORDER ) { | ||
return; | ||
} | ||
|
||
if ( position.y === 'top' && index > fromIndex ) { | ||
props.onDrop( uid, index - 1 ); | ||
return; | ||
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. Rather 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. Absolutely :-) |
||
} else if ( position.y === 'bottom' && index < fromIndex ) { | ||
props.onDrop( uid, index + 1 ); | ||
return; | ||
} | ||
|
||
props.onDrop( uid, index ); | ||
} catch ( err ) { } | ||
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 need 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.
|
||
} | ||
}; | ||
|
||
return ( | ||
<DropZone | ||
onFilesDrop={ dropFiles } | ||
onDrop={ reorderBlock } | ||
/> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ import { | |
stopTyping, | ||
updateBlockAttributes, | ||
toggleSelection, | ||
moveBlockToIndex, | ||
} from '../../store/actions'; | ||
import { | ||
getBlock, | ||
|
@@ -63,6 +64,7 @@ import { | |
isTyping, | ||
getBlockMode, | ||
} from '../../store/selectors'; | ||
import { BLOCK_REORDER } from '../../store/constants'; | ||
|
||
const { BACKSPACE, ESCAPE, DELETE, ENTER, UP, RIGHT, DOWN, LEFT } = keycodes; | ||
|
||
|
@@ -101,19 +103,23 @@ export class BlockListBlock extends Component { | |
this.maybeStartTyping = this.maybeStartTyping.bind( this ); | ||
this.stopTypingOnMouseMove = this.stopTypingOnMouseMove.bind( this ); | ||
this.mergeBlocks = this.mergeBlocks.bind( this ); | ||
this.insertBlocksAfter = this.insertBlocksAfter.bind( this ); | ||
this.reorderBlock = this.reorderBlock.bind( this ); | ||
this.onFocus = this.onFocus.bind( this ); | ||
this.onPointerDown = this.onPointerDown.bind( this ); | ||
this.onKeyDown = this.onKeyDown.bind( this ); | ||
this.onBlockError = this.onBlockError.bind( this ); | ||
this.insertBlocksAfter = this.insertBlocksAfter.bind( this ); | ||
this.onTouchStart = this.onTouchStart.bind( this ); | ||
this.onClick = this.onClick.bind( this ); | ||
this.onDragStart = this.onDragStart.bind( this ); | ||
this.onDragEnd = this.onDragEnd.bind( this ); | ||
|
||
this.previousOffset = null; | ||
this.hadTouchStart = false; | ||
|
||
this.state = { | ||
error: null, | ||
dragging: false, | ||
}; | ||
} | ||
|
||
|
@@ -348,6 +354,98 @@ export class BlockListBlock extends Component { | |
this.setState( { error } ); | ||
} | ||
|
||
onDragStart( event ) { | ||
const dragInset = document.getElementById( `block-drag-inset-${ this.props.uid }` ); | ||
const block = document.getElementById( `block-${ this.props.uid }` ); | ||
|
||
// Closure to remove the cloned node from the DOM (fired within timeout below) | ||
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 is it fired with a 0 timeout? 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. This fires after the |
||
const removeBlockClone = ( blockList, cloneWrapper ) => { | ||
return () => { | ||
blockList.removeChild( cloneWrapper ); | ||
}; | ||
}; | ||
|
||
// Closure to hide the visible block and show inset in its place (fired within timeout below) | ||
const showInset = () => { | ||
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. Do we need this to be a function that returns a function? 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. Nope - at least not in its current state. It was accepting parameters initially, but not since I switched to updating state here. I will refactor this eventually, just not sure right now if a state variable is the way to go here - I do notice some inconsistencies in IE, which I need to investigate. 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. 👍 |
||
return () => { | ||
this.setState( { dragging: true } ); | ||
document.body.classList.add( '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. Does this piece of information need to be kept at 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 am not sure and I still need to work this out - it currently doesn't work as I like. Whatever I seem to try, the handler just changes randomly to "copy" in the middle of dragging sometimes. 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 does this happen? Also, what have you tried? 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. Hey, I will focus on this as my next in line task. It's been sitting in my backlog for some time to be specific right now, but I will share details (or solution!) soon 😃 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, should be fixed now - had to add a fake drag image to avoid browser defaults with dragging (Chrome changes the cursor apparently). Similar issue to what these folks seem to be having: SortableJS/Sortable#246 I've added Any other concerns/suggestions on this? |
||
}; | ||
}; | ||
|
||
event.dataTransfer.setData( | ||
'text', | ||
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 I drag an image block to my desktop I get a JSON file with this internal representation. Low-priority, but I am curious whether we can somehow affect that. 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. Nice catch! I will investigate. |
||
JSON.stringify( { | ||
uid: this.props.uid, | ||
fromIndex: this.props.order, | ||
type: BLOCK_REORDER, | ||
} ) | ||
); | ||
|
||
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. In what cases this won't be true? 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. This block makes 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.
Oh man, don't ask. IE11!
Totally agree this will be refactored in the end. |
||
const blockList = block.parentNode; | ||
const cloneWrapper = document.createElement( 'div' ); | ||
const clone = block.cloneNode( true ); | ||
const blockRect = block.getBoundingClientRect(); | ||
const blockTopOffset = parseInt( blockRect.top, 10 ); | ||
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. Indentation here seems a bit off. 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. Thanks - spotted a few other linting errors too, but should be cleared in latest commit. Just run the linter and all green. 😃 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. 👍 |
||
const blockLeftOffset = parseInt( blockRect.left, 10 ); | ||
|
||
// 1. Clone the current block, style it, and spawn over original block. | ||
|
||
clone.id = `clone-${ block.id }`; | ||
|
||
cloneWrapper.classList.add( 'editor-block-list__block-clone' ); | ||
// 40px padding for the shadow. | ||
cloneWrapper.style.width = `${ parseInt( blockRect.width, 10 ) + 40 }px`; | ||
// Position clone right over the original block. | ||
// 20px padding for the shadow. | ||
cloneWrapper.style.top = `${ blockTopOffset - 20 }px`; | ||
cloneWrapper.style.left = `${ blockLeftOffset - 20 }px`; | ||
|
||
cloneWrapper.appendChild( clone ); | ||
blockList.appendChild( cloneWrapper ); | ||
|
||
// 2. Set clone as drag image and remove from DOM: | ||
|
||
// Display the drag image right over the block: | ||
// - Coordinates relative to the block and the cursor/ | ||
// - Optimisation: if top ends of the block are outside the viewport, | ||
// then revert to top 0 (relative to cursor). | ||
const top = blockTopOffset > 0 ? | ||
parseInt( event.clientY, 10 ) - blockTopOffset + 20 : | ||
parseInt( event.clientY, 10 ) + 20; | ||
|
||
event.dataTransfer.setDragImage( | ||
cloneWrapper, | ||
parseInt( event.clientX, 10 ) - blockLeftOffset + 20, | ||
top | ||
); | ||
|
||
setTimeout( removeBlockClone( blockList, cloneWrapper ), 0 ); | ||
} | ||
|
||
// Hide the visible block and show inset in its place. | ||
setTimeout( showInset(), 0 ); | ||
} | ||
|
||
onDragEnd() { | ||
const block = document.getElementById( `block-${ this.props.uid }` ); | ||
const dragInset = document.getElementById( `block-drag-inset-${ this.props.uid }` ); | ||
|
||
const resetBlockDisplay = () => { | ||
return () => { | ||
this.setState( { dragging: false } ); | ||
document.body.classList.remove( 'dragging' ); | ||
}; | ||
} | ||
|
||
setTimeout( resetBlockDisplay(), 0 ); | ||
} | ||
|
||
reorderBlock( uid, toIndex ) { | ||
this.props.moveBlockToIndex( uid, toIndex ); | ||
} | ||
|
||
render() { | ||
const { block, order, mode, showContextualToolbar, isLocked } = this.props; | ||
const { name: blockName, isValid } = block; | ||
|
@@ -360,13 +458,17 @@ export class BlockListBlock extends Component { | |
// Generate the wrapper class names handling the different states of the block. | ||
const { isHovered, isSelected, isMultiSelected, isFirstMultiSelected, focus } = this.props; | ||
const showUI = isSelected && ( ! this.props.isTyping || ( focus && focus.collapsed === false ) ); | ||
const { error } = this.state; | ||
const { error, dragging } = this.state; | ||
const wrapperClassName = classnames( 'editor-block-list__block', { | ||
'has-warning': ! isValid || !! error, | ||
'is-selected': showUI, | ||
'is-multi-selected': isMultiSelected, | ||
'is-hovered': isHovered, | ||
'is-reusable': isReusableBlock( blockType ), | ||
'is-hidden': 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 this a concern of the consumer of 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 is correct - showing an overlay in place of the original content or some other style deviation is the concern of the consumer. |
||
} ); | ||
const blockDragInsetClassName = classnames( 'editor-block-list__block-drag-inset', { | ||
'is-visible': dragging | ||
} ); | ||
|
||
const { onMouseLeave, onFocus, onReplace } = this.props; | ||
|
@@ -381,6 +483,7 @@ export class BlockListBlock extends Component { | |
/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */ | ||
return ( | ||
<div | ||
id={ `block-${ this.props.uid }` } | ||
ref={ this.setBlockListRef } | ||
onMouseMove={ this.maybeHover } | ||
onMouseEnter={ this.maybeHover } | ||
|
@@ -391,9 +494,37 @@ export class BlockListBlock extends Component { | |
onClick={ this.onClick } | ||
{ ...wrapperProps } | ||
> | ||
<BlockDropZone index={ order } /> | ||
{ ( showUI || isHovered ) && <BlockMover uids={ [ block.uid ] } /> } | ||
{ ( showUI || isHovered ) && <BlockSettingsMenu uids={ [ block.uid ] } /> } | ||
<div | ||
id={ `block-drag-inset-${ this.props.uid }` } | ||
draggable={ true } | ||
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: we can drop the |
||
onDragStart={ this.onDragStart } | ||
onDragEnd={ this.onDragEnd } | ||
className={ blockDragInsetClassName } | ||
> | ||
<div className="inner" ></div> | ||
</div> | ||
|
||
<BlockDropZone | ||
index={ order } | ||
onDrop={ this.reorderBlock } | ||
/> | ||
|
||
{ ( showUI || isHovered ) && | ||
<BlockMover | ||
draggable={ true } | ||
onDragStart={ this.onDragStart } | ||
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. Do these events need to be applied to every one of the child components? Can't we add a single drag event to the root wrapper component? If it's an issue of competing with 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. Arghh, I wish there was an easier way to accomplish this whole effect. The way I have designed the logic to handle dragging is we need to apply these methods to everything that we want to use as a drag handle (a point that the user can grab something from). They will behave the same way regardless of where they are applied. This way we don't have to experiment with an overlay that will interfere with other drag logic (i.e. resizing an image, selecting text, or anything else). So to answer your question: these events |
||
onDragEnd={ this.onDragEnd } | ||
uids={ [ block.uid ] } | ||
/> | ||
} | ||
{ ( showUI || isHovered ) && | ||
<BlockSettingsMenu | ||
draggable={ true } | ||
onDragStart={ this.onDragStart } | ||
onDragEnd={ this.onDragEnd } | ||
uids={ [ block.uid ] } | ||
/> | ||
} | ||
{ showUI && isValid && showContextualToolbar && <BlockContextualToolbar /> } | ||
{ isFirstMultiSelected && <BlockMultiControls /> } | ||
<div | ||
|
@@ -407,6 +538,7 @@ export class BlockListBlock extends Component { | |
tabIndex="0" | ||
aria-label={ blockLabel } | ||
> | ||
|
||
<BlockCrashBoundary onError={ this.onBlockError }> | ||
{ isValid && mode === 'visual' && ( | ||
<BlockEdit | ||
|
@@ -469,6 +601,7 @@ const mapDispatchToProps = ( dispatch, ownProps ) => ( { | |
onSelect() { | ||
dispatch( selectBlock( ownProps.uid ) ); | ||
}, | ||
|
||
onDeselect() { | ||
dispatch( clearSelectedBlock() ); | ||
}, | ||
|
@@ -488,6 +621,7 @@ const mapDispatchToProps = ( dispatch, ownProps ) => ( { | |
uid: ownProps.uid, | ||
} ); | ||
}, | ||
|
||
onMouseLeave() { | ||
dispatch( { | ||
type: 'TOGGLE_BLOCK_HOVERED', | ||
|
@@ -519,9 +653,14 @@ const mapDispatchToProps = ( dispatch, ownProps ) => ( { | |
onMetaChange( meta ) { | ||
dispatch( editPost( { meta } ) ); | ||
}, | ||
|
||
toggleSelection( selectionEnabled ) { | ||
dispatch( toggleSelection( selectionEnabled ) ); | ||
}, | ||
|
||
moveBlockToIndex( uid, index ) { | ||
dispatch( moveBlockToIndex( uid, index ) ); | ||
}, | ||
} ); | ||
|
||
BlockListBlock.className = 'editor-block-list__block-edit'; | ||
|
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.
Why a blanket
additionalProps
, instead of listing the specificdiv
props here? Also, you might want to update the component'sREADME.md
, too.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.
tbh I've seen this pattern used a couple of places and thought to reuse it here e.g.
https://github.com/WordPress/gutenberg/blob/master/components/icon-button/index.js#L24
https://github.com/WordPress/gutenberg/blob/master/components/placeholder/index.js#L12
https://github.com/WordPress/gutenberg/blob/master/components/button/index.js#L43
https://github.com/WordPress/gutenberg/blob/master/components/external-link/index.js#L18
but definitely no strong feelings about it. I think you are right, I should enumerate the new params. I will update the README too 😃