Support drag and drop in horizontal block lists and improve drop zone detection #23020
Conversation
talldan
added
[Type] Bug
talldan
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz and
youknowriad
as code owners
talldan
force-pushed
the
try/better-block-drop-detection
branch
from
c096ae4
to
64080a6
Compare
|
Size Change: +209 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
docs/designers-developers/developers/data/data-core-block-editor.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is really cool :) Thanks for working on this.
Ping @jasmussen as he's been working on drag and drop recently too
Ping @ellatrix as she was working on horizontal in between inserters which can be related.
|
Thank you for working on this, this is really cool! This is what I see:
Interestingly, the Navigation Block has that "absorb block toolbar" prop that means child blocks have their toolbar aligned to the parent block. Which is good, except for when you drag and drop :D then it gets a little weird. Something to think about for the quote block for when that gets child blocks actually, cc: @ellatrix @mtias — we might not want to absorb the child toolbar after all in that block. Probably fine for Navigation, Buttons and Social Links still. I left a small comment, but this feels pretty good to me. @talldan you may want to have a look at #23024 for what we might end up doing for drag and drop — as far as I can tell it doesn't affect things here, but just as an FYI. |
|
Thanks for all the feedback. 😄
@jasmussen Yep, I think @tellthemachines spotted that too. Not sure if there's a ticket for it ... possibly not as I think the drag handle and movers were in the process of being changed.
I can't see any overlap between the PRs, but I can double-check, it'll be nice to see the two changes working together. |
talldan
force-pushed
the
try/better-block-drop-detection
branch
from
bfafbdc
to
ebb1a1a
Compare
|
I've added some unit tests and JSDocs, so this is ready for review now. I'm not entirely sure I wrote all the |
| moverDirection: getBlockListSettings( targetRootClientId ) | ||
| ?.__experimentalMoverDirection, |
There was a problem hiding this comment.
If this is merged, I think __experimentalMoverDirection could be renamed in a follow-up to something like blockOrientation, as it's now being used more widely than the movers.
There was a problem hiding this comment.
How about blockListOrientation or blockFlowOrientation? I agree moverDirection doesn't really fit anymore.
talldan
force-pushed
the
try/better-block-drop-detection
branch
from
ebb1a1a
to
25fe575
Compare
There was a problem hiding this comment.
I tested this in Firefox and it works really well. Nice work! 🎉
I noticed just two things, neither are blocking:
-
It can be challenging to drag a block to the bottom of the editor—I have to flail my cursor around like a madman to find the drop zone. It's much better than it is in
master, though. -
I can't ever drag a Column block to the beginning or end of a Columns block. You can't do this at all in
master, though.
I think let's merge this early so that it gets some incidental testing by Gutenberg developers before the next plugin release.
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
| } ); | ||
|
|
||
| return candidateIndex; | ||
| } |
There was a problem hiding this comment.
That was probably the most readable code that deals with geometry I've ever seen! ❤️
| // Flip the elementData to make a horizontal block list. | ||
| const horizontalElements = elementData.map( mapElements( 'horizontal' ) ); | ||
|
|
||
| describe( 'getNearestBlockIndex', () => { |
There was a problem hiding this comment.
You get a 🌮 for adding tests!
Co-authored-by: Robert Anderson <robert@noisysocks.com>
Yeah, it's still not great, but then I'm not sure it was working at all before. The problem is that as soon as the mouse leaves the drop zone element (which is the One way to solve it could be to make the entire editor canvas as a drop zone, and make It might be a bit detrimental to performance, but could be worth trying. |
|
Does this PR change anything with regards to scrolling and dragging? #23082 |
|
@jasmussen The two PRs don't have any overlap, they should work together well. |
|
This is great, thanks for working on it. For the main issue @jasmussen highlights we'll need the ability to drag from the block itself (initially only when in select mode). |
|
@talldan Do you think any e2e tests could be created for drag and drop? |
|
@ellatrix Yep, I can give it a go. I imagine it'd be possible to use |
|
Yes, there's some prior work on dragging in the multi selection e2e tests. Also resizing the spacer block I think. |
| ( sourceRootClientId === '' && | ||
| targetRootClientId === undefined ); |
There was a problem hiding this comment.
@talldan: Could we converge on the types of these two expectably similar variables? If that's not possible, can we add comments explaining the discrepancy?
Description
Fixes #22662
This PR adds support for drag and drop in horizontal block lists (buttons, social links, navigation) and also improves how we detect where the user intends to drop a block.
Creating a draft PR for now as this needs tests, but it'd be good to get some feedback.
How does this work
Previously, block drag and drop worked by using the
yposition of the dragged item, and comparing that to theyposition of the blocks being dragged over. When theyposition was less than halfway over a block, the block would be dropped before, and when more than halfway dropped after.For horizontal block lists there are more challenges, this system can't be translated to the
xaxis because horizontal block lists tend to wrap onto multiple lines (for example, the buttons block).This PR instead compares the position of the dragged item to the relevant edges of blocks. The top and bottom edges for vertical blocks lists and the left and right edges for horizontal. It picks the edge of the block that's nearest to the dragged item and uses that to determine where the item should be dropped.
Alternatives
I considered a few different alternatives and also tried a few different ways to calculate the drop position. One idea was drawing invisible block drop zones over the block list that can more concretely act as a drop target. This didn't work too badly, but I'd consider it a bigger change and it would also have issues if something has a higher z-index than the drop zone.
Remaining issues
There are still some areas that can be improved:
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: