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

Conversation

chriskmnds
Copy link
Contributor

@chriskmnds chriskmnds commented Dec 20, 2017

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?

  • Specs written for the new reducer function.
  • UX tests only on desktop and manually at this point.
  • Some E2E tests fail currently. I will work these out last, after the prototype/design is finalised.

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):

dnd-blocks-dragging

Long blocks with height greater than 700px get transformed (scaled down 50%) when cloned:

dnd-blocks-transformation

The underlay div mentioned below:

dnd-underlay

The inner underlay that depicts the actual gray area shown while dragging:

dnd-blocks-underlay-inner

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:

  1. On drag start, the block to be reordered gets cloned and the transfer data is set. The clone is styled and positioned right over the original block (this manages to set the visible block along with a shadow as the drag preview that we see when dragging). The original block is hidden and the underlay's inner div becomes visible so its grey background is shown in place of the block.
  2. On drag over (dragging) the clone's coordinates are updated relative to the cursor position as the mouse moves.
  3. On drag end, the clone is removed from the DOM and the block's visibility is reset. The underlay's inner div is set back to invisible.
  4. On drop, if within a dropzone, the reducer function is called to reindex and update the blocks list.

Browser Compatibility

Currently tested with latest:

  • Mac/Chrome
  • Mac/Firefox
  • Mac/Safari
  • Windows10/IE11

Unless otherwise stated in the issues below, functionality is consistent across these browsers.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

Known Issues:

List of issues to address/investigate. Some of these address @jasmussen's and @nb's comments below.

  • Update inset (gray area) color to $light-gray-200.
  • Update inset area to match block boundaries.
  • Update cursor to grab while hovering and grabbing while dragging. Investigate IE/Edge fallback/polyfill.
  • Investigate inconsistencies with cursor styling (sometimes shows the default or "copy" handle instead of "grabbing" when dragging).
  • Update/confirm design for narrow views.
  • Investigate adding a shadow and opacity to the drag preview.
  • Investigate behavior with long blocks that span multiple pages.
  • Confirm cross-browser compatibility.
  • Investigate a bug when sometimes the hidden block's editable content re-appears while dragging (particularly when dragging something off the screen).
  • Communicate better the availability of dragging to the user (turn ellipsis into a drag handler, add another menu item, etc.).
  • Workaround for IE's lack of setDragImage support. Invent a solution that does not deviate too much from the rest. Keep in mind the bug/issue with onDrag in Safari here: https://bugzilla.mozilla.org/show_bug.cgi?id=505521
  • Investigate issues with Chrome in narrow/mobile desktop view. It does not get the correct block offset to spawn the clone at the right place. Not an issue with the other browsers and not an issue in wider desktop view in Chrome.
  • Update README of dropdown component (and any other component that new properties were added) + avoid the ...additionalProps blanket in places that it was used.
  • Fix issue with blue line indicators when dragging a block over a dropzone. This is a more general issue with the dropzones, not just for reordering. I have created a ticket that addresses this more generally: 4329
  • Fix BUG with long block lists. This happens when we drag and scroll downwards past a long blocks list (e.g. 20 blocks long). The block clone is never removed from DOM unless we refresh the page.
  • Verify functionality with latest master changes and the sibling inserter tweaks. Reconfirm design with @jasmussen. (Haven't noticed any visual regression or style changes, so marking this complete.)
  • Review use of timeouts in the the block component.
  • Investigate situation with nested blocks. This does not work on nested blocks currently and does not fail gracefully either. This is a major blocker.
  • Investigate possibility to enable dragging from the ellipsis and block movers without passing the drag handlers to them.
  • Review usage (and location) of BLOCK_REORDER constant set in data transfer during drag operation.
  • Investigate failing e2e tests and redesign accordingly.
  • Scroll page while dragging over the top/bottom edges of the screen. This is currently using the browser defaults and is not 100% consistent -> Safari defaults to NO automatic scrolling while dragging.

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)
  • The image gallery dropzone indicators become visible while dragging. Might not be an issue, but worth investigating an easy fix.
  • Investigate behavior in long documents (e.g. 20 pages long). Is scrolling sufficient for this case, or should we explore some other angle? A thought that comes to mind is to keep a reference in a small box fixed at the bottom of the page on dragEnd (given some condition), so a user can pull it back and continue dragging. Condition may be dropping the block clone outside a dropzone while initial block is not within the viewport.
  • Investigate behavior when dropping a block outside the page i.e. on the desktop -> it creates a JSON file with the transfer data. @nb was wondering if we can somehow affect that.
  • Investigate possibility of a reusable component that can be used for image galleries as well.
  • Investigate mobile (touch) support.

- 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.
@jasmussen
Copy link
Contributor

You posted a screencast in Slack that's looking really good. Very nice work!

Some questions:

  • The purple area illustrated in the screenshots above, is that the draggable area? If so, cool! That's bigger than I'd expected.
  • Can this perform in crappy browsers like IE11? (We need to sort of support both IE11 and Edge, and if we can't we have to at least fail gracefully)
  • How does this perform on really long documents? Like 20 pages?
  • How does this perform if you drag to the edge of the screen, does it start scrolling the screen? Both directions?
  • Can this ever be made to work on mobile? Note, this is not a requirement as we have other tools for rearranging content, and it would likely be a fiddly affair regardless. But it would be nice to know if it's a functionality we can build on top of this in the future.

A few wishlist items for the implementation of block drag and drop:

  • We have some color variables you can use. The "inset" area should be $light-gray-100, but that's looking a little light in the screencast, can you try $light-gray-200?
  • The gray inset area covers the entire area of the block, including the margins all around. I understand this is probably difficult to address, but ideally the gray area would cover only the same area that the block boundary covers.
  • Probably difficult for the same reason the previous item is difficult — you mentioned in Slack some difficulties with the drop shadow. Can you elaborate?
  • Can you tweak the cursors a bit? When you hover over the draggable area, the cursor should be cursor: grab;, and when you're dragging, the cursor should be cursor: grabbing;. You can use cursor: move; as a fallback, I'm not sure if IE/Edge supports those CSS3 cursors.

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.

@chriskmnds
Copy link
Contributor Author

chriskmnds commented Dec 21, 2017

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 illustrated in the screenshots above, is that the draggable area? If so, cool! That's bigger than I'd expected.

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.

Can this perform in crappy browsers like IE11? (We need to sort of support both IE11 and Edge, and if we can't we have to at least fail gracefully)

I haven't tried yet, but definitely aim for this to support the browsers in our list or fail gracefully.

How does this perform on really long documents? Like 20 pages?

How does this perform if you drag to the edge of the screen, does it start scrolling the screen? Both directions?

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 😁

Can this ever be made to work on mobile? Note, this is not a requirement as we have other tools for rearranging content, and it would likely be a fiddly affair regardless. But it would be nice to know if it's a functionality we can build on top of this in the future.

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.

We have some color variables you can use. The "inset" area should be $light-gray-100, but that's looking a little light in the screencast, can you try $light-gray-200?

No problem! I just used F8F8F8 in the prototype.

The gray inset area covers the entire area of the block, including the margins all around. I understand this is probably difficult to address, but ideally the gray area would cover only the same area that the block boundary covers.

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.

Probably difficult for the same reason the previous item is difficult — you mentioned in Slack some difficulties with the drop shadow. Can you elaborate?

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 😃

Can you tweak the cursors a bit? When you hover over the draggable area, the cursor should be cursor: grab;, and when you're dragging, the cursor should be cursor: grabbing;. You can use cursor: move; as a fallback, I'm not sure if IE/Edge supports those CSS3 cursors.

Nit a problem. Will look into this.

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.

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.

@chriskmnds
Copy link
Contributor Author

chriskmnds commented Dec 21, 2017

Thanks again for the wonderful feedback Joen! ❤️ I will keep you posted on progress, share a few more screencasts, etc. as I carry on. 😎

@jasmussen
Copy link
Contributor

Awesome!

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

Sometimes when I need to animate something, like a shadow appearing, I apply an invisible box shadow to the element, always there, like box-shadow: 0 0 0 0 $dark-gray-500; along with a transition: box-shadow .2s;. Then when I need the animation to fire, I can just animate the existing box shadow, a la &:hover { box-shadow: 0 0 30px 0 $dark-gray-500; and it won't flicker because "the shadow was always there".

Might that work? In any case, not critical, but something to think about.

@chriskmnds
Copy link
Contributor Author

Sometimes when I need to animate something, like a shadow appearing, I apply an invisible box shadow to the element, always there, like box-shadow: 0 0 0 0 $dark-gray-500; along with a transition: box-shadow .2s;. Then when I need the animation to fire, I can just animate the existing box shadow, a la &:hover { box-shadow: 0 0 30px 0 $dark-gray-500; and it won't flicker because "the shadow was always there".

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.
@chriskmnds chriskmnds changed the title Rearrange blocks via drag & drop (v2. using editor dropzones). Reorder blocks via drag & drop (v2. using editor dropzones). Dec 22, 2017
- 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.
…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.
@youknowriad
Copy link
Contributor

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 👍

package.json Outdated
@@ -39,7 +39,7 @@
"querystringify": "1.0.0",
"re-resizable": "4.0.3",
"react": "16.2.0",
"react-autosize-textarea": "2.0.0",
"react-autosize-textarea": "3.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to be added here?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@@ -208,6 +208,29 @@ export function replaceBlock( uid, block ) {
return replaceBlocks( uid, block );
}

/**
* Returns an aciton object signalling that an indexed block should be moved
Copy link
Contributor

Choose a reason for hiding this comment

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

s/aciton/action/


// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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 );
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?

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.

onDragStart={ this.onDragStart }
onDragEnd={ this.onDragEnd }
isDragging={ dragging }
elementId={ blockElementId }
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 *' );
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 this for? Does it need to be applied as wildcard to all its children? Or is the container enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know :) cc @chriskmnds

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

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see if we can simplify this to eliminate the wildcard selector.

@@ -345,6 +412,10 @@
position: absolute;
top: 0;
padding: 0;

/* Necessary for drag indicator */
Copy link
Member

Choose a reason for hiding this comment

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

Necessary in what way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for showing the grab handle when hovering over the area.

@@ -422,6 +493,8 @@

.editor-block-list .editor-inserter {
margin: $item-spacing;
cursor: move;/* Fallback for IE/Edge < 14 */
cursor: grab;
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

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least, sounds like a good inline code comment.

At best, we ought not need to sprinkle these all around the stylesheets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how to express this honestly :) Some help would be appreciated.

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

Copy link
Member

Choose a reason for hiding this comment

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

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' );
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 need this? What happens if it's not here?

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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -452,6 +465,7 @@ export class BlockListBlock extends Component {
'is-multi-selected': isMultiSelected,
'is-hovered': isHovered,
'is-reusable': isReusableBlock( blockType ),
'is-hidden': 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 this a concern of the consumer of Draggable ? Seems to require a lot of work to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


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


return (
<DropZone
onFilesDrop={ this.onDropFiles }
Copy link
Member

Choose a reason for hiding this comment

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

   
onDrop onDrop
onHTMLDrop onHTMLDrop
onFilesDrop ... onDropFiles ?

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.

@youknowriad youknowriad force-pushed the add/drag-n-drop-for-blocks--dropzone branch from 8b939a4 to 7dcc860 Compare March 30, 2018 14:42
@youknowriad youknowriad force-pushed the add/drag-n-drop-for-blocks--dropzone branch from 2c7106c to b20b033 Compare March 30, 2018 14:50
@chriskmnds
Copy link
Contributor Author

chriskmnds commented Mar 30, 2018

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

grabbing-from-controls

@mtias
Copy link
Member

mtias commented Apr 2, 2018

This one needs a final rebase. Also, can we make it so that the new BlockTitle component also works as a drag handler?

@youknowriad
Copy link
Contributor

Rebased, I tried adding a Draggable handler around BlockTitle but failed for now because the title disappears when you click it which prevents the dragStart from triggering. Not certain how to solve this at the moment.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

Duplicate heading.

function MyComponent() {
return (
<DropZoneProvider>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Is the intermediate div necessary here?

aduth
aduth previously requested changes Apr 3, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

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.

@youknowriad youknowriad dismissed stale reviews from aduth and nb April 3, 2018 11:13

It should be fine

@youknowriad youknowriad merged commit c0ee73a into WordPress:master Apr 3, 2018
@youknowriad
Copy link
Contributor

youknowriad commented Apr 3, 2018

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

@jasmussen
Copy link
Contributor

HOOOLY GUACAMOOOOLEEEEE 🔥 🔥 🔥

@@ -48,6 +48,7 @@ class DropZoneProvider extends Component {
window.addEventListener( 'dragover', this.dragOverListener );
window.addEventListener( 'drop', this.onDrop );
window.addEventListener( 'mouseup', this.resetDragState );
this.container = findDOMNode( this );
Copy link
Member

Choose a reason for hiding this comment

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

We should have suppressed this warning with a valid reason for disabling.

Aside: IMO we shouldn't have ESLint warnings; only errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants