Cover block: Pass dropZoneElement reference to fix dragging within cover block area #56312
Conversation
andrewserong
added
[Type] Bug
|
Size Change: +12 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewserong
changed the title
|
Thanks for giving this a look!
It's a slightly odd case — because this component renders in a number of different states (placeholder vs "real") it's a bit simpler to use state and pass a setState function to the For now, though, this PR is just a proof of concept — I'm not 100% sure if this is exactly what I'll go with for this PR in the end, but it seemed a good way to prove out the |
…ing within cover block area
andrewserong
force-pushed
the
try/fix-cover-block-dropzone
branch
from
b117ebe
to
599bea2
Compare
After re-testing, it turns out I was overthinking the This PR is now a single-line code change, which is nice. I haven't fully tested it manually yet, but I think this is probably a good direction for it now, so I'll switch it over to "Ready for review". I'll do some more manual testing on it tomorrow 🙂 |
andrewserong
changed the title
There was a problem hiding this comment.
Thanks for the update! Much cleaner change now 🎉
Tested in post and site editor and everything is working as expected: blocks can more easily be dragged into a Cover block by hovering over its empty areas, and they can still be dragged out of a Cover block by hovering over its edges. LGTM!
|
Thanks for the approving review! 🙇 After a bit more testing, I think this isn't quite the right solution just yet. While it makes it much easier to drag within a cover block, unfortunately it feels a bit too greedy to use the entire area of the block for the drop zone, as it means it can then be impossible to drag between adjacent blocks when there's no space between them (as can sometimes happen in site designs that intentionally have Group or Cover blocks right next to each other with contrasting background colors). On trunk, if you drag into the top area of the cover block, then the drop indicator line goes between the blocks: 2023-11-23.14.07.50.mp4In this PR, dragging moves between first child of the Cover block, or last child of the preceding Group block, with no way to drop in between: 2023-11-23.14.06.15.mp4Ideally, I think we want something where the block's dropZoneElement is used as the drop zone, but there's a bit of breathing room around it, so that if you're within a threshold (i.e. I think in the end we'll still likely end up with a PR that looks like this one (passing in the |
I noticed that in testing, but given that it already happens between two adjacent Group blocks, I didn't see it as an issue 😄 |
Ah, gotcha! Well, in that case, I'll try merging this one in, then. It's an easy revert if need be, and I'll open up a separate issue for investigating seeing if we can add a threshold to force in-between when we're very close to the edge of a block. I have a sneaking suspicion we might be able to figure something out in getDropTargetPosition or thereabouts, but it'll likely need a fair bit of hacking around 😀 For now, and since either way we'll still want to provide a reference to the wrapper for the Cover block, I'll merge this in. Thanks for the encouragement! |
|
I've opened up a follow-up issue in #56463, so merging in now 👍 |
I guess that's a side effect of how every block list is a drop zone, the group's drop zone completely overlaps it's parent's drop zone. Another way to explain it might be that the various drop zones aren't working together to create a completely cohesive system. 🤔 I think I did try at one point implementing only a single dropzone at the root for all blocks (similar to how list view works), but it didn't work too well. There are some edge cases like Media and Text where only a portion of the block should be a block drop zone, so you do need something that declares where blocks can be dropped. |
|
Thanks for the extra context, Dan! That points to handling it explicitly within the |
|
Update: I think it'll be possible to do it based on calculations in |
What?
Note: this PR started out as a proof of concept that we can fix #26049 via passing a drop zone element.
Related to: #26049, following on from #56070
Pass in a
dropZoneElementfor the wrapper of the Cover block to fix dragging within a cover block's area.Why?
Without providing a
dropZoneElementfor the wrapper of the cover block, dragging within the padding of the cover block is treated as dragging either before or after the Cover block instead of within it. This makes it hard to drag something intentionally within a cover block.How?
ref's current value used for the cover wrapper touseInnerBlocksProps'dropZoneElementpropTesting Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before
2023-11-20.15.11.59.mp4
After
2023-11-20.15.31.55.mp4