Allow dragging between adjacent container blocks based on a threshold #56466
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +292 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Update: still very hacky, but I've gotten the drop indicator line to now reflect the threshold so it feels a bit more stable now: 2023-11-24.15.42.44.mp4Will continue hacking on the remaining to-do items next week! |
|
Nice experiment! From testing it feels like one has to linger a little too long on top of the border between the two adjacent containers in order to get the indicator to appear, especially when moving upwards. draggydrop.movAlso the visibility of the indicator could be a bit better; I think the problem is we're overlaying a line-shaped indicator on an existing line (the border between blocks) and it's really easy to miss. Not sure exactly how it could look otherwise. Perhaps there could be some animation on the draggable element when it hovers over a dropzone? |
|
Thanks for taking this for an early spin!
Oh, good point. Out of curiosity, were you dragging between two adjacent Cover blocks, or a Group block and a Cover block? From the screengrab, it looks like the bottom-most block might be a Group block, which doesn't yet use the
Definitely a lot we can explore with visual feedback as a user drags over or between dropzones to provide an immediate signal of a change, rather than having to wait for the animation to complete. It'll be a good one to play around with, but possibly for another PR? |
This might also be helped by adjusting the size of the threshold area, too 🤔 |
It was a Cover and a Group! Sounds like that was the problem, the lag was definitely greater when moving up than down.
Yeah that's probably out of scope for this PR; I think the pressing thing for now is to fix the inability to drop between containers! |
andrewserong
force-pushed
the
try/threshold-drag-between-blocks
branch
from
b453c39
to
0bf25a4
Compare
Update: yep, looks like that was the problem! Thanks again for testing, I've pushed an update to the Group block and that appears to have resolved it now, so it's a lot easier to get to that in-between position: 2023-11-28.16.46.27.mp4There's still a bit of work to do in resolving some edge cases before this will be ready for review — things like checking that the parent block's orientation is vertical, otherwise it can get a little odd if a Cover block is the child of a Row block (maybe not terrible, but still odd): 2023-11-28.16.51.12.mp4It'll also be interesting to see if the addition of the threshold interferes for anyone when the block they're working with intentionally doesn't have much in the way of padding within it, or if it's quite a small or short block 🤔 In any case, I'll continue working through those things, but very happy for feedback from anyone along the way! |
|
Just surfacing a little context from this other PR where we have a mockup for hovering on top of a block to surface dropzones before and after: #56186 (comment) Notably, there's a mockup here that might benefit this particular PR (in addition to the conversation itself). |
Oh, nice, thank you for the mockup! At this stage, I think this PR will likely be valuable to implement as a separate step prior to seeing if we can shift elements around to make space for the before and after positions. That is to say, if first we can improve increasing the area within which a valid drop target is made, the hopefully it'll be a little more straightforward in follow-ups to then explore how we present that valid drop target. I have a bit more work to do on this PR before it'll be ready for review (I've been a bit more focused on list view displacement so far this week), but I'll ping for more feedback when this is ready for a proper look 🙂 |
packages/block-editor/src/components/use-on-block-drop/index.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in b1e0fad. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7177202242
|
andrewserong
changed the title
|
Update: I believe I've fixed all the edge cases I encountered during testing, so I think this should be ready for review and/or feedback now. It doesn't take care of dragging between all adjacent blocks, just container blocks with padding (i.e. Cover and Group), but the hope is that this should still make a difference to the ease of dragging while building or making adjustments to templates, since it's pretty common to have adjacent covers and groups. |
|
Hrm, looks like the |
|
Okay, I think I've fixed the test in ab292fd by adding a minimum height before this threshold behaviour can be used. I've set it to Also if we're happy with the direction of this PR, it might be worth us seeing if we can add another e2e test to cover this change, too, if it isn't too difficult to reproduce the desired behaviour in an e2e. |
| @@ -20,6 +20,9 @@ import { | |||
| } from '../../utils/math'; | |||
| import { store as blockEditorStore } from '../../store'; | |||
|
|
|||
| const THRESHOLD_DISTANCE = 30; | |||
| const MINIMUM_HEIGHT_FOR_THRESHOLD = 120; | |||
There was a problem hiding this comment.
So, if a block is 120px high, its "inner" dropzone will be 60px high?
Is this an arbitrary min height or does it correspond to some average height for new blocks?
The computed height for a new, empty group block is 48px in my browser.
I guess it makes more sense to want to drop things inside an empty group block rather than around it 😄
There was a problem hiding this comment.
I guess it makes more sense to want to drop things inside an empty group block rather than around it 😄
That was my thinking. These are just arbitrary numbers for now, but in practice, I figured you probably wouldn't want the threshold for the top/bottom positions being active unless there was at least the same amount of room on the inside of the block to allow for "real" internal dragging. So 120 seemed like a reasonable default to go with for now as it's 4 * the threshold distance. Happy to change any of these values, of course!
|
Just been testing in Chrome, FF and Safari in LTR and RTL locales. Dragging and dropping between container elements is more intuitive in my experience. 🎉 Before2023-12-08.14.41.15.mp4After2023-12-08.14.38.35.mp4I removed padding on a few group blocks and dropping in between and inside works as I'd expect. It's a bit fiddly to often get things to land where you want, but it's also this way on trunk. 2023-12-08.14.46.35.mp4 |
| rootBlockIndex = 0, | ||
| } = options; | ||
|
|
||
| if ( dropZoneElement && parentBlockOrientation !== 'horizontal' ) { |
There was a problem hiding this comment.
interesting that this doesn't seem to be a problem with blocks in horizontal containers even if they have zero space between them (in my testing the inserter always works)
There was a problem hiding this comment.
Actually spoke too soon 😅 it can be a problem if trying to insert a block between two Stacks inside a Row.
There was a problem hiding this comment.
Oh, good point! We could always expand the logic here. Do you mind sharing a video if you get a chance? I'll continue working on this next week!
There was a problem hiding this comment.
Here's a screengrab of trying to insert a Heading between two Stacks nested inside a Row. You can only really see the drop indicator when hovering at the end of the Row, and that takes a little doing:
drag-in-row.mov
There was a problem hiding this comment.
Oh, yes, that is pretty challenging that one!
I suppose two potential options here:
- Add in handling for the horizontal orientation here, too, and see if that resolves it (i.e. use threshold on the horizontal axis if the parent is horizontal), or:
- Treat this PR as vertical only for now, and look at horizontal in a potential follow-up
I'm happy to go either way, but I'll have a play around with horizontal blocks next week and give it a little more thought 🙂
There was a problem hiding this comment.
Either option sounds good! Anything we do here will be an improvement on the current state of things 😄
| const onBlockDrop = useOnBlockDrop( | ||
| dropTarget.operation === 'before' || dropTarget.operation === 'after' | ||
| ? parentBlockClientId | ||
| : targetRootClientId, |
There was a problem hiding this comment.
So this works, but I can't quite figure out how 😅 because at root level, targetRootClientId should be undefined. But I can drop a Paragraph between two Images at root level, with 0 block spacing, very smoothly!
Also I tried logging the operation here while dragging and it always switches to insert before the drop happens. I guess that's expected because it's actually an insertion?
There was a problem hiding this comment.
So this works, but I can't quite figure out how 😅 because at root level, targetRootClientId should be undefined.
I think the comment for this hook is a bit misleading, unfortunately! At the root of the document, targetRootClientId will be undefined / an empty string, however for a container block at the root of the document that calls useBlockDropZone, it'll pass its clientId in as rootClientId here:
What this means, is that for a container block, by the time you're inside useBlockDropZone, targetRootClientId refers to the client id of the container block itself. So for our purposes, we need to pass in parentBlockClientId (the parent of the container's clientId) in order for it to work properly. In practice, for a Group block at the root of the document, targetRootClientId will be the client id of the Group block, and parentBlockClientId will be an empty string, as we're at the root of the document.
Hope that helps, I know it's quite a mouthful! If / when this PR looks ready to land, I'll see if I can write this up as a comment prior to this line so that it's clearer — and / or so that we can remember what we did here 😄
Also I tried logging the operation here while dragging and it always switches to insert before the drop happens.
Oh strange! I didn't notice that, but the drop should be happening with the previously returned onBlockDrop that had the last state with the "before" or "after" operation. When you perform a drop, I think it'll immediately clear out the dropTarget state back to its default which is insert, so in practice while logging we might be observing the returning to the previous state 🤔
I'm possibly either over thinking that part (or slightly confused about it), but in practice, before, after, and insert should all do the same thing, as the only operation that does anything differently in useOnBlockDrop is replace.
If it's causing any issues though, I can follow-up next week!
There was a problem hiding this comment.
Oooh I see, thanks for explaining! Yes a comment would be great.
If it's causing any issues though, I can follow-up next week!
It's not causing any issues that I could notice, so should be fine to leave as-is!
| @@ -124,6 +126,7 @@ function GroupEdit( { attributes, name, setAttributes, clientId } ) { | |||
| ? blockProps | |||
| : { className: 'wp-block-group__inner-container' }, | |||
| { | |||
| dropZoneElement: ref.current, | |||
There was a problem hiding this comment.
So in #56312 a similar change was made in Cover so that dragging over the empty area inside the block would make the inner drop zone show, and that caused the drop zone between blocks with no margins to be hidden. But this change is now necessary to make it possible for the in between drop zone to show between Groups?
My main question is how this may affect third party containers - will all containers now need a dropZoneElement ref passed to useInnerBlocksProps?
There was a problem hiding this comment.
Good question!
My main question is how this may affect third party containers - will all containers now need a dropZoneElement ref passed to useInnerBlocksProps?
With this PR (and right now) I think they might need to if they want to allow the threshold dragging behaviour in this PR. It's a tricky one, because we can't really know exactly what a 3rd party block is doing with its UI and where its inner blocks will be placed in relation to anything else displayed within the block itself (a bit like how we have things like Media & Text, which is a non-standard kind of container block).
I think I'd lean slightly toward guarding the behaviour in this PR behind the dropZoneElement behaviour for now (and having it be manually passed to useInnerBlocksProps) while we're still fine-tuning how all the drag and drop behaviour works. Then, perhaps further down the track, we could look at making it all happen automatically within useInnerBlocksProps somehow? The tricky part in thinking about that, though, is that not all blocks might be using a ref attached to their outer wrapper, so it might be hard to make it all automatic. Still, worth thinking about!
There was a problem hiding this comment.
we can't really know exactly what a 3rd party block is doing with its UI and where its inner blocks will be placed in relation to anything else
Yeah this is a good point. In testing with plain WP 6.4 I notice that it's also not possible to drop between two Columns blocks, but it is between Media & Text blocks. Those behaviours are unaltered in this PR so I guess that means regressions are unlikely?
I'm still not sure I quite grasp why the same thing that made dropping between blocks stop working for Cover in #56312 (passing the ref to useInnerBlocksProps) is making dropping between Group blocks work in this PR 😅
There was a problem hiding this comment.
I'm still not sure I quite grasp why the same thing that made dropping between blocks stop working for Cover in #56312 (passing the ref to useInnerBlocksProps) is making dropping between Group blocks work in this PR 😅
It's because we're doing it at the same time as the rest of this PR 😄
If we did a standalone PR that passed dropZoneElement to the Group block's useInnerBlocksProps then we'd expose the issue that we can't drag between two adjacent Group blocks, because the two blocks' dropZoneElements would be adjacent to each other, without any threshold for drag-between. But because this PR now enables a drag threshold within the dropZoneElement, the Group block gets to skip the intermediate step where we break the ability to drag between adjacent blocks.
There was a problem hiding this comment.
It's already broken though - I guess the difference is that on trunk a drop indicator appears, apparently between the blocks, but in reality dropping over it just adds the dropped block to one of the Groups.
This happens in WP 6.4 with or without Gutenberg:
drop-between-groups.mov
There was a problem hiding this comment.
Gotcha! Yep, on trunk, I think you're right, it looks like it's just dropping within 👍
I must admit, I do find it hard to maintain a good mental model for how all this hooks together 😄
Thanks again for giving this a look!
There was a problem hiding this comment.
I must admit, I do find it hard to maintain a good mental model for how all this hooks together
Same 😭
…dropping at the same level
andrewserong
force-pushed
the
try/threshold-drag-between-blocks
branch
from
ab292fd
to
b1e0fad
Compare
|
Update: I've added logic so that if the parent block is horizontal (i.e. it's a Row block) then we use the threshold on the left/right edges. Seems to be working pretty well! 2023-12-12.15.51.32.mp4With that in place, I think this PR might be ready for a final review now 🤞 |
|
Thanks for reviewing! I'm sure we'll have more to do to tweak the behaviour here, but I'll merge this in now so we can explore further tweaks in follow-ups. (Also, if we run into any showstoppers in real world usage, it should be pretty straightforward to revert if need be) |
What?
Fixes #56463
This PR explores a potential fix for dragging between adjacent container blocks (specifically Group and Cover blocks) when dragging within their padding area.
The proposal here is to add a threshold area (within
30px) of the vertical and horizontal edges of the container block, where if a user drags in that area, we attempt to drop before or after the container block rather than within it.Why?
If you have a template with adjacent Group or Cover blocks where there is no gap between those blocks, it is either very difficult or impossible to drag between them. This PR explores one potential solution to it, by adding a threshold area to allow dragging between these blocks (in the vertical and horizontal axes)
How?
dropZoneElementand the user is dragging toward the top or bottom of the block (i.e. within a30pxthreshold) then treat the drag/drop as being intended for before or after the block instead of within it.120px)dropZoneElementfor the Group block so that its container element can be used to determine where to place the drop.Note: the behaviour in this PR is most noticeable / mostly applies when a Group block has padding set. The Cover block already uses a padding area, so it's quite noticeable with the Cover block, too.
To-do
Ensure the parent block allows drag to the position (i.e. what if the user shouldn't be allowed to break out of the current block in this context)Out of scope, as this is tracked in: [drag-and-drop] Visual cue for dropping element shown in another block's inner content even when dropping isn't possible #24174Testing Instructions
In a post or template (the TwentyTwentyFour template is good for testing this) attempt to drag and drop a block between two adjacent container blocks (in particular Cover and Group blocks). With this PR applied, you should be able to drag just within the padding area (or
30pxof it) of the block to drag between the adjacent blocks. Note: it's expected that Group blocks without any padding should not receive this threshold, as the user will be dragging over a child of the Group block.Test the above, but with Row as a parent block, and set the Row's block spacing to
0so there are no gaps. If the children are Cover blocks or Group blocks with some padding, then the threshold behaviour in this PR should apply. That is, it should now be easier to drag between adjacent blocks. The logic should also work correctly in RTL languages.See if this adversely affects drag or performance in any way compared to
trunk.Screenshots or screencast
Vertical
The following screengrab shows dragging between adjacent Cover and Group blocks, where dragging just toward the vertical edge allows for dragging between the blocks.
2023-11-28.16.46.27.mp4
Horizontal (LTR language)
2023-12-12.15.51.32.mp4
Horizontal (RTL language)
2023-12-12.16.10.16.mp4