Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 66 commits
Commits
Show all changes
84 commits
Select commit Hold shift + click to select a range
b62160a
Drag & Drop: Added functionality for reindexing blocks.
chriskmnds Dec 20, 2017
7723d70
Drag & Drop for blocks: Added drag listeners and styling for dragging…
chriskmnds Dec 20, 2017
51cd0f5
Drag & Drop for blocks: Updated drop logic to improve precision of th…
chriskmnds Dec 21, 2017
82e9886
Drag & Drop for blocks: Resolved conflicts with latest master.
chriskmnds Dec 22, 2017
d6df73d
Drag & Drop for blocks: Added z-index to block underlay used in dragg…
chriskmnds Dec 27, 2017
f38db58
Drag & Drop for blocks: Changed background color of block inset/underlay
chriskmnds Dec 27, 2017
1b04b5b
Drag & Drop for blocks: Fixed visible grayed inset area when dragging.
chriskmnds Dec 27, 2017
9f25b10
Drag & Drop for blocks: Removed unnecessary block container element.
chriskmnds Dec 27, 2017
0a23609
Drag & Drop for blocks: Updated cursor when hovering/dragging a
chriskmnds Dec 27, 2017
9f3b3af
Drag & Drop for blocks: Added z-index property to placeholder compone…
chriskmnds Dec 28, 2017
52ac1ff
Drag & Drop for blocks: Updated dragging inset margin to be effective…
chriskmnds Dec 28, 2017
c55ebc3
Merge branch 'master' into add/drag-n-drop-for-blocks--dropzone
chriskmnds Dec 29, 2017
6c7a913
Drag & Drop for blocks: Fixed issues that surfaced since merging late…
chriskmnds Dec 30, 2017
8dbc69e
Drag & Drop for blocks: Some cleanup - renamed 'underlay' to 'drag-in…
chriskmnds Dec 30, 2017
35f9923
Drag & Drop for blocks: Changed dataTransfer data type for moving blo…
chriskmnds Dec 30, 2017
9271649
Drag & Drop for blocks: Improved conditioning for onDrop handler.
chriskmnds Jan 4, 2018
8fdcd6e
Drag & Drop for blocks: Added a small margin between cursor and drag
chriskmnds Jan 4, 2018
1e2ae5e
Drag & Drop for blocks: Removed browser prefix for cursor style.
chriskmnds Jan 4, 2018
ec5052c
Drag & Drop for blocks: IE11 tweaks. Added conditional call to
chriskmnds Jan 4, 2018
62afc6d
Drag & Drop for blocks: Some styling cleanup.
chriskmnds Jan 5, 2018
8f13212
Drag & Drop for blocks: IE11/Safari tweaks.
chriskmnds Jan 6, 2018
883ab19
Drag & Drop for blocks: Updated logic for setting drag image.
chriskmnds Jan 7, 2018
6faceef
Drag & Drop for blocks: Updated drag image logic to be consistent across
chriskmnds Jan 9, 2018
4978348
Drag & Drop for blocks: Updated drag image shadows and styling.
chriskmnds Jan 13, 2018
b512271
Drag & Drop for blocks: Updated logic for controlling the visibility of
chriskmnds Jan 14, 2018
e2c2e61
Drag & Drop for blocks: Some cleanup after code review.
chriskmnds Jan 16, 2018
8436f21
Drag & Drop for blocks: Added more draggable handles to the block, in…
chriskmnds Jan 17, 2018
f131740
Drag & Drop for blocks: Some cleanup.
chriskmnds Jan 17, 2018
2599d42
Drag & Drop for blocks: Updated cursor to move/grab for the block-set…
chriskmnds Jan 18, 2018
6ad1c44
Drag & Drop for blocks: Cleanup from previous commit - set the cursor…
chriskmnds Jan 18, 2018
6fc98de
Drag & Drop for blocks: Updated dragging logic to move clone around i…
chriskmnds Jan 19, 2018
178cc60
Drag & Drop for blocks: Some cleanup - removed logging, and added a h…
chriskmnds Jan 19, 2018
c98ea9c
Drag & Drop for blocks: Cleared linting errors.
chriskmnds Jan 19, 2018
6c2bf61
Drag & Drop for blocks: Updated drag start/end handlers to stop propa…
chriskmnds Jan 21, 2018
ab68b11
Drag & Drop for blocks: Added a fake/invisible drag image to avoid
chriskmnds Jan 21, 2018
702eec3
Drag & Drop for blocks: Added transformation to the block clone if
chriskmnds Jan 22, 2018
7cbdfde
Drag & Drop for blocks: Some code cleanup.
chriskmnds Jan 22, 2018
b0f0b0e
Drag & Drop for blocks: Updated block-mover and dropdown components to
chriskmnds Jan 23, 2018
43ec29a
Drag & Drop for blocks: Some cleanup - inline comments, new lines, etc.
chriskmnds Jan 28, 2018
ec53f4f
Drag & Drop for blocks: Making sure the DOM is not updated prior to r…
chriskmnds Feb 9, 2018
2e0b13a
Drag & Drop for blocks: Moved drag init/end logic to a higher order
chriskmnds Feb 12, 2018
a2246f4
Drag & Drop for blocks: Cleanup.
chriskmnds Feb 12, 2018
db7a34b
Drag & Drop for blocks: Resolved conflicts with master. Several updat…
chriskmnds Feb 12, 2018
d8bedee
Drag & Drop for blocks: Cleanup. Removed leftover styles.
chriskmnds Feb 12, 2018
25485f9
Drag & Drop for blocks: Code cleanup.
chriskmnds Feb 13, 2018
05f2d10
Drag & Drop for blocks: Code cleanup. Further extracted drag init/end
chriskmnds Feb 13, 2018
f33d4db
Drag & Drop for blocks: Resolved conflicts.
chriskmnds Feb 13, 2018
a83c738
Drag & Drop for blocks: Updated cursor styling to grab for the new bl…
chriskmnds Feb 13, 2018
30ee308
Drag & Drop for blocks: Updates after PR feedback.
chriskmnds Feb 24, 2018
da001c9
Drag & Drop for blocks: Some cleanup after PR feedback. removed unnec…
chriskmnds Feb 24, 2018
b071cc4
Drag & Drop for blocks: Updated cursor styling after PR feedback to g…
chriskmnds Feb 24, 2018
46ed7a2
Drag & Drop for blocks: Updated index of reusable block edit panel to…
chriskmnds Feb 25, 2018
129dac9
Drag & Drop for blocks: Some cleanup after PR feedback. Renamed _stat…
chriskmnds Feb 25, 2018
d6897ce
Drag & Drop for blocks: Added support for inner blocks.
chriskmnds Mar 4, 2018
4e66a4a
Drag & Drop for blocks: Cleanup - lint errors.
chriskmnds Mar 4, 2018
bfb78c1
Drag & Drop for blocks: Updates after PR review. No longer passing drag
chriskmnds Mar 10, 2018
0a6a05d
Drag & Drop for blocks: Got rid of BLOCK_REORDER constant and selector.
chriskmnds Mar 10, 2018
f72e05d
Drag & Drop for blocks: Updates and cleanup after PR review.
chriskmnds Mar 10, 2018
f03e4ec
Drag & Drop for blocks: Cleanup. Removed commented out code.
chriskmnds Mar 10, 2018
650f613
Merge remote-tracking branch 'origin/master' into add/drag-n-drop-for…
youknowriad Mar 23, 2018
5c1d339
Drag And Drop: Refactor using a component instead of Higher Order Com…
youknowriad Mar 23, 2018
5c1613d
Clarify Drop Events
youknowriad Mar 23, 2018
4d3b8b8
Fix dragging between nested and not nested blocks
youknowriad Mar 23, 2018
d731edf
Fix Drag Area and Styling
youknowriad Mar 23, 2018
9624dca
More cleaning and fix insert position
youknowriad Mar 23, 2018
94dd1cc
Extract BlockDraggable Component
youknowriad Mar 23, 2018
7f01121
Destructre the uid block prop
youknowriad Mar 26, 2018
9982bcd
Clarify BlockDraggable classnames
youknowriad Mar 26, 2018
5623a71
Clarify Draggable Component Docs
youknowriad Mar 26, 2018
f48d65a
Avoid creating a new block object if the layout didn't change when mo…
youknowriad Mar 26, 2018
d415a1f
Less generic body className
youknowriad Mar 26, 2018
0eca12c
Bind BlockDropZone event handlers to avoid rerenderings
youknowriad Mar 26, 2018
e763e28
Remove Block UI on the cloned block element which fixes drag and scroll
youknowriad Mar 26, 2018
f19dcd0
Decrease the opacity of the cloned draggable
youknowriad Mar 26, 2018
48f8087
Updating ReactTextareraAutosize to fix a bug on initial load (long po…
youknowriad Mar 26, 2018
2759481
Remove iframes from the clone to fix embed's drag and drop
youknowriad Mar 26, 2018
51c1cef
Fix multiple dropzones showing up in Gallery block
youknowriad Mar 27, 2018
794dfd0
Merge remote-tracking branch 'origin/master' into add/drag-n-drop-for…
youknowriad Mar 30, 2018
4a817c5
Changes per review
youknowriad Mar 30, 2018
7dcc860
Fix scrolling when dragging
youknowriad Mar 30, 2018
b20b033
Remove useless styles
youknowriad Mar 30, 2018
c61c770
Merge remote-tracking branch 'origin/master' into add/drag-n-drop-for…
youknowriad Apr 2, 2018
d5daf30
Fix wide blocks scroll
youknowriad Apr 3, 2018
a648f67
Fix typos in documentation
youknowriad Apr 3, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions components/draggable/README.md
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.
Copy link
Member

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".


## 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.
Copy link
Member

Choose a reason for hiding this comment

The 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`
152 changes: 152 additions & 0 deletions components/draggable/index.js
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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we check typeof ?

...And as a follow-up, should we note via code comment that it's for browser compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the IIFE? Shouldn't dragImage still be accessible from the outer scope?

Copy link
Contributor Author

@chriskmnds chriskmnds Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onDragStart is an event handler. There could be a case (albeit highly unlikely) where the event fires again before document.body.removeChild has been called. So both calls will end up happening on the latter dragImage constant in that case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So both calls will end up happening on the latter dragImage constant in that case.

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

Copy link
Contributor Author

@chriskmnds chriskmnds Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are of course absolutely right. That would only make sense if dragImage was in some outer/global scope. Not sure if it ever was either. Well caught. :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 is the default value for the second argument and could be omitted:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout

Copy link
Contributor Author

@chriskmnds chriskmnds Mar 23, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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() {
Copy link
Member

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like .remove is supported in IE11: https://caniuse.com/#feat=childnode-remove

Elsewhere in the project, .parentNode.removeChild is used.

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 );
19 changes: 19 additions & 0 deletions components/draggable/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
body.dragging {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a .dragging class unique enough to avoid potential conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not - need to think of a better name here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the className to use is-dragging-components-draggable Let me know if you think of a better name.

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' );
}
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export { default as CodeEditor } from './code-editor';
export { default as Dashicon } from './dashicon';
export { DateTimePicker, DatePicker, TimePicker } from './date-time';
export { default as Disabled } from './disabled';
export { default as Draggable } from './draggable';
export { default as DropZone } from './drop-zone';
export { default as DropZoneProvider } from './drop-zone/provider';
export { default as Dropdown } from './dropdown';
Expand Down
1 change: 1 addition & 0 deletions components/placeholder/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
width: 100%;
max-width: 280px;
flex-wrap: wrap;
z-index: z-index( '.components-placeholder__fieldset' );

p {
font-family: $default-font;
Expand Down
11 changes: 11 additions & 0 deletions edit-post/assets/stylesheets/_z-index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 '.editor-block-list__block-drag-inset' above to confirm?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's needed :) for the gray background when dragging 🤕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The z-index itself is not really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Absolutely. I didn't mean for my answer to excuse my clumsiness in adding these - I just didn't know any better. :-)

Copy link
Contributor Author

@chriskmnds chriskmnds Mar 30, 2018

Choose a reason for hiding this comment

The 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?

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.

'.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,
Expand Down
30 changes: 28 additions & 2 deletions editor/components/block-drop-zone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand Down Expand Up @@ -53,10 +53,32 @@ function BlockDropZone( { index, isLocked, ...props } ) {
}
};

const onDrop = ( event, position ) => {
Copy link
Member

Choose a reason for hiding this comment

The 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;
Copy link
Member

Choose a reason for hiding this comment

The 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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming doesn't seem consistent between the two properties – dropFiles and onDrop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I could rename onDrop to something like reorderBlock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: When does onDrop occur? On its name alone, I'd consider for any drop, files or HTML included. But based on the separation of props, it seems more a fallback callback. The docs are very unclear (and by unclear, I mean non-existent).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

/>
);
}
Expand Down Expand Up @@ -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;
Expand Down
31 changes: 31 additions & 0 deletions editor/components/block-list/block-draggable.js
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 } ) {
Copy link
Member

Choose a reason for hiding this comment

The 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 isDragging, since this state can be known by the callbacks provided through its own Draggable element.

Copy link
Contributor

@youknowriad youknowriad Mar 23, 2018

Choose a reason for hiding this comment

The 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 isDragging is not local state. I actually thought about using Redux to track but decided against it to avoid useless actions, selectors...

Copy link
Member

Choose a reason for hiding this comment

The 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', {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing just draggableClassname at this point should suffice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just className or classes. "Draggable" can already be assumed to be known by the file / component name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, this is no longer in block.js. Sorry, many changes.

'is-visible': isDragging,
} );

const transferData = {
type: 'block',
rootUID: rootUID,
fromIndex: index,
uid: uid,
layout,
Copy link
Member

Choose a reason for hiding this comment

The 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. uid and rootUID qualify as well.

};

return (
<Draggable className={ blockDragInsetClassName } transferData={ transferData } { ...props }>
<div className="inner" ></div>
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're expecting extending Draggables to render the child, should this be offered as a component by the Draggable ? Draggable.Inner ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space before opening > should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not certain I understand the purpose of a Draggable.Inner component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of <div className="editor-block-list__block-draggable-inner"></div> ?

</Draggable>
);
}

export default BlockDraggable;
Loading