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

Support live drag and drop #16457

Closed
wants to merge 3 commits into from
Closed

Support live drag and drop #16457

wants to merge 3 commits into from

Conversation

youknowriad
Copy link
Contributor

An alternative to #15741

This PR introduces live drag and drop. Instead of moving blocks when we drop them, this PR moves the block as we move them. The result is more fluidity.

@youknowriad youknowriad added the [Feature] Drag and Drop Drag and drop functionality when working with blocks label Jul 8, 2019
@youknowriad youknowriad requested review from mtias and oandregal July 8, 2019 09:49
@youknowriad youknowriad self-assigned this Jul 8, 2019
@youknowriad youknowriad requested a review from jasmussen July 8, 2019 09:50
@@ -12,6 +12,8 @@ import isShallowEqual from '@wordpress/is-shallow-equal';
const { Provider, Consumer } = createContext( {
addDropZone: () => {},
removeDropZone: () => {},
getDragData: () => {},
setDragData: () => {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For security reasons browsers don't allow the dropzones to access the "dragdata" content until the drop event is triggered. So this is a way to keep track of the data while dragging. Since we control both the draggable and the droppable, there's no issue in doing so.

break;
case 'default':
dropZone.onDrop( event, position );
}

dropZone.onUniversalDrop( normalizedDragEvent, position );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically not useful in this PR, but I find the three different callbacks per data type to be a bad API. this tries to introduce a non-breaking change API to replace all three callbacks with a normalized callback equivalent to the newly introduced onDragOver one.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 8, 2019

Choose a reason for hiding this comment

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

The fact that we pass onFilesDrop or onHTMLDrop means that the component user supports the dropping of a file or HTML.
This information is used then to render visual indication when the dragging is possible and if it isn't the drop events are not even triggered. If we follow the onUniversalDrop we will need some flags/an array specifying what kind of drops are supported, or the component parent needs to pass a callback that says if a given drag is possible.

@jasmussen
Copy link
Contributor

Nice work. Here's a GIF:

dragondrop

There are some things we should discuss, but before that and on a high level: this is impressive work, and it has the DNA of something amazing. I can't wait to see how this ends.


There's a smallish issue that is performance related: appears to be some jank when moving blocks short distances. It feels like it's related to some hover labels maybe/maybe not blinking in and out of existance as you are moving. I'm sure we can fix this.


The larger issue is one of the actual drag and drop behavior itself. The thing you see in master can prosaically be described like this: when you start dragging, you lift the block off the page and see a gray slot where it used to be, and a blue line indicator for where you mean to drop it.

This design came to be out of the idea that this drag and drop helps clarify the intention of the action — you can see where the block was (gray slot), and where it will be when you release (blue line).

The other classic drag and drop method is more real time. As soon as you start dragging a block, it is taken out of the flow and now sits under your cursor until you drop it.

What's in this branch right now sits kind of in between those two approaches — blocks are moved immediately when a "destination slot" is under the cursor, but you have no clone, and no blue line to indicate where you can drop the block. So, although it feels more organic, it's harder to see where to drop the block. Surprisingly I don't miss the clone — but the lack of the blue line means that when your mouse is not over a different dropzone, it looks like drag and drop is broken:

unclear

I would suggest that the best thing we can do here is pick sides.

Option 1, keep the blue line. Either we keep the blue destination indicator (maybe maybe make it even thicker?), but add in your lovely animation and ditch the clone (trying in this branch the clone itself does not seem necessary). But it also probably means you can't move the block until you release the cursor — i.e. no fluid "live movement".

Kind of like this:

option 1

Option 2, embrace the fluidity entirely. That means as soon as you start dragging, you pull the block out of the flow — if your mouse was a hand that hand now holds the block and holds it until you release it, and surrounding blocks rearrange themselves as you move it around.

Kind of like this (from react-beautiful-dnd):

option 2

I used to be a proponent of the blue line approach due to the intrinsic way it let you know what would happen if you released the cursor. But I've come around the the more fluid approach (example 2 above) because it feels more like actual drag and drop, and because we have highly visible undo controls if you made a mistake. But it feels like it's either or — either the blueline indicates where you drop the thing and THEN the block is moved, or the block is pulled out of the flow and makes room for itself regardless of where the mouse is. Combining the two, I'm not sure can work.

What do you think?

@mtias
Copy link
Member

mtias commented Jul 8, 2019

@jasmussen this is still very much in flux. I think it needs the following things present (closer to this):

  • Lift the block but constrain to the axis in which it sits.
  • Be less conservative with the threshold of when a block is swapped (right now it requires getting closer to passing the next block).
  • Don't show the gray area.

We also need to check the preferred flow for nested areas.

@mtias mtias added the [Status] In Progress Tracking issues with work in progress label Jul 8, 2019
@jasmussen
Copy link
Contributor

Be less conservative with the threshold of when a block is swapped (right now it requires getting closer to passing the next block).

I think that is the key missing ingredient. So long as there's a big "dead area" where the mouse doesn't appear to do anything, it feels buggy. Agree that https://codesandbox.io/embed/01p1kxymow is something that could work really well.

@mtias
Copy link
Member

mtias commented Jul 8, 2019

Yes, exactly. It's also interesting how the expectation changes when you remove what you are dragging. I think this might actually expose that the threshold was ill-conceived from the start, making it all feel a bit more sluggish.

@jasmussen
Copy link
Contributor

I think this might actually expose that the threshold was ill.conceived from the start, making it all feel a bit more sluggish.

Show me a sword and I will throw myself on it 😅

@youknowriad
Copy link
Contributor Author

youknowriad commented Jul 8, 2019

I tweaked the drop zone. Now it moves the block as soon as you touch another block.

@youknowriad
Copy link
Contributor Author

I spent the day on this. The bad news is that it's not possible to use the approach from that codepen because it relies on the fact that the array of data doesn't really reorder while in our case it's a requirement.

I think we can build on top of the current approach to get close to that behavior but I'm not sure we'd able to recreate it entirely.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

This PR looks like a promising step, nice work @youknowriad.
The code changes look good and in the right direction. I also did not notice any editor crashes during the tests.
I noticed the following UI issues:

  • While dragging blocks, I'm not able to see the content of the block until the drag finishes, seeing the block live would be nice.
  • If I add a column or a group block at the end of the document, it is tough to move another block to be after it.
  • Dragging a block over the media placeholder causes the placeholder to move a lot:
    Jul-08-2019 19-28-19
  • Dragging a file to a media placeholder is very hard:
    Jul-08-2019 19-28-14
  • Dragging a file (e.g., image) without a media block being previously inserted does not work, and there is no visual feedback on the action.
  • It would be nice to increase the width of the draggable block area to include the margin at the sides.

if ( event.type !== 'default' ) {
return;
}
this.moveBlock( event.data );
Copy link
Member

Choose a reason for hiding this comment

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

Calling moveBlock results in a call to this.props.moveBlockToPosition being made, this really modes the block in the store and creates an undo level. We need to make sure all these undo actions are aggregated, otherwise, when moving a block the user may create lots of undesired undo levels.

break;
case 'default':
dropZone.onDrop( event, position );
}

dropZone.onUniversalDrop( normalizedDragEvent, position );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Jul 8, 2019

Choose a reason for hiding this comment

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

The fact that we pass onFilesDrop or onHTMLDrop means that the component user supports the dropping of a file or HTML.
This information is used then to render visual indication when the dragging is possible and if it isn't the drop events are not even triggered. If we follow the onUniversalDrop we will need some flags/an array specifying what kind of drops are supported, or the component parent needs to pass a callback that says if a given drag is possible.

case 'file':
dropZone.onFilesDrop( [ ...event.dataTransfer.files ], position );
dropZone.onFilesDrop( [ ...normalizedDragEvent.files ], position );
Copy link
Member

Choose a reason for hiding this comment

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

Should we verify if dropZone.onFilesDrop is defined now that we have onUniversalDrop?

@mtias
Copy link
Member

mtias commented Jul 14, 2019

because it relies on the fact that the array of data doesn't really reorder while in our case it's a requirement.

Is it a requirement to reorder the array before dropping the block?

@SchneiderSam
Copy link

Short feedback from an end user's point of view: very cool. That's really great!

@paaljoachim
Copy link
Contributor

Can we get a status update here?
This is a feature that users will very much notice.
Thanks!

@youknowriad
Copy link
Contributor Author

The current state of this PR is not perfect. Especially, it doesn't work well in nested blocks. I've spent a lot of time thinking about this and It's not clear that this is a better alternative than what we having a "target" indicator to allow the user to pick properly where he wants to drop the dragged block. The problem with live drag and drop is that it's very hard to know where to drop the block if we hover two blocks (nested) at the same time. I'm going to close this PR right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Drag and Drop Drag and drop functionality when working with blocks [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants