Support both horizontal and vertical in-between inserters #27860
Conversation
|
Size Change: +774 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
This is cool! I'm testing in TwentyTwentyOne Blocks and seeing some issues around how the buttons are spaced, this appears to be a separate issue, but it does affect the positioning of the sibling inserter:
In this branch the sibling inserter is also not perfectly centered between blocks, in the horizontal version. Those are some things we'll probably want to hammer out to get this shipping, but in principle, this seems fine! Anything in specific you'd like input on? |
|
Yes, I'm not sure exactly how to solve the positionning issue when the position is supposed to change (after inserting a button for instance, you'll notice a jump). I'd like some input from @ellatrix here on how you think we should deal with the Popover position properly. And yes, you notice that it's not centered properly neither horizontally or vertically between two adjacent blocks, the reason is "margins". Margins between blocks are random values defined by the CSS of the theme and it's not something we can easily include in the computation of the position of the in-between inserter. It seems the current approach was just relying on the default top/bottom margin values. |
|
Yep. Margins and space-between properties make this one tricky, I can definitely see it. Outside of being able to calculate the space between blocks, sans margins or space-between shenanigans, I'd be open to creative solutions. |
youknowriad
force-pushed
the
try/vertical-inbetween-inserter
branch
2 times, most recently
from
d0dcf85 to
e9423ab
Compare
|
I pushed some tweaks to center the insertion point properly in both directions (by dynamically getting the position of the previous block). There's still some funkiness when clicking the inserter though. |
|
This works great when I just hover — I see the insertion guide and + button and it all lines up as expected for both horizontally and vertically orientated containers. But once I add a block, the insertion guide remains. Here's what it looks like when I try to add a new Column:
The newly added column doesn't have a width set, so it's a little funky to actually use; It would be nice to give it a defined width and adjust the others accordingly. After adding the new Column the in-between inserter no longer works anywhere and gets stuck. I can recreate a similar behavior when adding a new Button in-between two existing buttons. |
youknowriad
force-pushed
the
try/vertical-inbetween-inserter
branch
from
e9423ab to
7640ad4
Compare
|
Made an update here, I think this is getting very close. |
|
(The columns width thing is something we should explore separately, it's not specific to the in between inserter, you can have the same result with the default inserter) |
|
Working great with Columns and Buttons, but I can't seem to trigger the in-between inserter within the Navigation block. |
|
@shaunandrews there are actually two bugs preventing the in-between inserter to trigger in the navigation block:
|
There was a problem hiding this comment.
@youknowriad Apologies it took so long to review. I know @ellatrix made some changes in #27537 to make the insertion point show vertically for drag and drop. But the changes here seem good, and I'm sure can work with the changes in that PR. I left a couple of questions that might need to be resolved.
In testing this is working well for me. I noticed that because the height of blocks when shown horizontally tends to be much less than the width for vertical blocks, things can get a bit cramped:
Not a blocker, I'm sure it can be iterated on over time.
packages/block-editor/src/components/block-list/insertion-point.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/insertion-point.js
Outdated
Show resolved
Hide resolved
| const targetRootClientId = clientId | ||
| ? getBlockRootClientId( clientId ) | ||
| : rootClientId; |
There was a problem hiding this comment.
I was wondering why the logic wasn't something like:
const targetRootClientId = rootClientId ? rootClientId : getBlockRootClientId( clientId );I'm sure there's a good reason, but might be worth leaving a comment for others reading the code.
There was a problem hiding this comment.
It has to do with the fact that this is both used for the insertion point when the inserter is opened (use the insertion poin t from there) and the hovered insertion point.
talldan
added
[Feature] Inserter
I just tested this again, because the screenshot looked a bit concerning. But after having tried it, yes, I'd agree this can be iterated — the important part is that the sibling inserter doesn't cover each block, you have to actually hover the pixels between the blocks. That's still a little fiddly, but it's better than the sibling inserter being too easy to invoke in such a small space, and still just something to look at. |
|
Thanks for the review @talldan both points were actual bugs that should be fixed now. |
youknowriad
force-pushed
the
try/vertical-inbetween-inserter
branch
from
f786a20 to
a7d4127
Compare
youknowriad
force-pushed
the
try/vertical-inbetween-inserter
branch
from
5e2bf71 to
c9f8abe
Compare
|
@kevin940726 I rebased, I think something in this PR actually broke the test but I don't understand what yet. |
|
I think the issue with the widgets tests is that now we need the previous and next blocks to render the insertion point while previously we were doing it with just the previous node. This means the insertion point doesn't show up when it's the last in the canvas, maybe it's fine for now. |
|
Ok let's try this and see how it goes. |
|
Hmm, why is this fine? This seems like a bug to me 🤔 . Don't we want to show a insertion point indicator when we're just hovering over a block in the global inserter? |
|
@kevin940726 I agree it's a good thing, though having the insertion point after the last element of the canvas, seemed less important than positioning the inserter properly between two adjacent blocks (basically this PR makes the next and previous mandatory to show the insertion point), I think it might be possible to have both but it may require some complexities in the code. |
|
@youknowriad If we agree that it should be supported, shouldn't we open a follow-up issue for that? Or is this already planned 😅? |
|
@kevin940726 Yes, good call, I tracked it here #28381 |
|
I'm not completely sure, but I believe this PR may have caused #28827. |
| const { ownerDocument } = containerRef.current; | ||
| const targetRect = target.getBoundingClientRect(); | ||
| const isReverse = clientY < targetRect.top + targetRect.height / 2; | ||
| const blockNode = getBlockDOMNode( clientId, ownerDocument ); | ||
| const container = isReverse ? containerRef.current : blockNode; | ||
| const closest = | ||
| getClosestTabbable( blockNode, true, container ) || blockNode; | ||
| const rect = new window.DOMRect( clientX, clientY, 0, 16 ); | ||
|
|
||
| placeCaretAtVerticalEdge( closest, isReverse, rect, false ); |
There was a problem hiding this comment.
I think we will want to bring this logic back though it probably needs adaptation for horizontal contexts. See #28682 (comment)
| @@ -550,36 +550,6 @@ describe( 'Writing Flow', () => { | |||
| expect( await getEditedPostContent() ).toMatchSnapshot(); | |||
| } ); | |||
|
|
|||
| it( 'should not have a dead zone between blocks (upper)', async () => { | |||
There was a problem hiding this comment.
Why did you remove this behaviour?
There was a problem hiding this comment.
I believe it's related to this #28682 (comment)
Basically, I just though that it doesn't make sense to focus the previous or the next block depending on a a small gap (the margin is very small) and just chose to focus the next one every time. Thought after discussing on that issue, I think focusing the end of the previous block or the start of the next block would be better.
This is a bit fragile as it's a bit of a hacky code on top of the existing code which was also a bit hacky.
There are two parts that are fragile:
1- Compute the currently hovered insertion point: This is based on a global mouse move event and the issue is globally we don't know whether the parent is vertical or horizontal forcing us to do some hacks.
2- Once we know that an inserter need to be shown, positionning it properly is very hacky as well. Previously, it was using the block element as
anchorRectof a Popover component and playing with the "position" (before or after the anchor). This approach don't work for horizontal popovers (positionning API is not good enough) so I've usedgetAnchorRectinstead and I compute the position. The issue is that the popover doesn't update the position at the right time, for instance when I insert a block, you'll notice that the position don't change instantly.Anyway, this kind of works, and might be polished but I wonder if we need to find a better way. This InsertionPointPopover seems very fragile to me. cc @ellatrix @talldan
Also, you'll notice that the insertion point is not centered properly between two blocks, the issue is that we don't really know the "margin" of the anchor to compute the position precisely.
Testing instructions