Make appender fixed position to avoid jumps in the UI #36605
Conversation
jasmussen
added
the
[Feature] Inserter
jasmussen
requested review from
shaunandrews,
youknowriad,
mtias,
jameskoster and
apeatling
|
Size Change: -441 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
This is definitely my favorite approach of the three. Having the appender appear inside the container makes it a lot clearer that it inserts into the block rather than after. The lack of appender when a single Button block is selected does feel off, but I'm not sure what the solution is there. I'm noticing a bug with the current implementation, though. The appender is not appearing for Group blocks. It is working fine with the Column and Buttons blocks: I also noticed this issue with the Buttons in-between inserter not acting as expected, but I don't know if that's related to this PR. It does compound with the lack of appender to make inserting Button blocks feel noticeably less ergonomic. If we later go the route of having appenders in the toolbar, perhaps one could be shown in the toolbar like so to provide a clear way of adding block to the parent of the current block: |
|
Thanks for the review!
Can you just verify if this isn't the same on trunk, though? Because I seem to recall a behavior where if you only have paragraphs inside a container, the appender isn't shown after either of them, whereas if you have, for example, an Image block inside, then it's shown. Definitely behavior to revisit in its own right, but just for the sake of this PR I think it's best to keep that separate.
I've been having a fair amount of problems with the that inbetween inserter lately, and I'm hoping we can get some help to look at it, including #35536. I don't know how this PR would affect it, though, as it's separate markup outside the canvas (popover) and shouldn't be subject to the CSS I've got here. I would agree the ergonomics aren't ideal here, and even just making the inserter work before and after the last blocks (illustrated here) could help bit. |
I specifically put a Heading block as the last block of the Group to see if that fixed the problem, but it didn't. I also just tried an Image block but it still didn't work. Just tested
Agreed. |
|
Really love the way this one is shaping up; Nice work. Visually, my only niggle is the placement of the [ + ] button and how it touches the focus outline of the block.
To me it feels unintentional, but I know the point is to visually connect the add button to the focused block. I think it would feel better if there was a little space (I used 3px) around the button, and let the existing context (things like the focus outline and block toolbar) tie the button to the container.
I think a good companion PR to this one could explore more explanation for where your block is going to be added. Right now, this PR can confuse adding blocks within deeply nested blocks, like a Buttons block within a Group block. Here's an example that uses a 3px blue "insertion guide" to help indicate where the new block will be added: Container.Inserter.Guide.mp4 |
With just a Paragraph and Heading block, I don't see a [ + ] for the Group — which I believe is intentional. When I add an Image block, then I do see the [ + ] button, which contradicts with your testing. I'm not sure what's going on, but I've tried it with more types of blocks and can reliably get the [ + ] when I expect it. |
|
@shaunandrews I'm using https://gutenberg.run for testing. Maybe there's an issue caused by its WP environment being a bit out-of-date? It is running WP 5.7 with Twenty Twenty-One 1.2. Changing the theme to Twenty Twenty or Twenty Nineteen does not fix the issue. I just tested the branch again and I still can't see the appender in a Group:
There are no console errors. Also, in In the pictures you posted above, the heading is the first block, but whether the appender is shown only depends on what the last child block is. |
jasmussen
force-pushed
the
try/alternate-fixed-appender
branch
from
5794f73 to
638830e
Compare
|
I do see the appender after the heading: I'm investigating a few thigns and will try and address the feedback noted. Whether we're able to land this one or not, I'd like to shepard it along as best I can. |
I do kind of like that it's as much on the edge as it can be, meaning it covers as little as possible of the content inside. Also that it lines up below. But this is not a strong opinion, and I'd be happy to add a few pixels of spacing if need be. Let me address the other things and I'll come back to this one. |
jasmussen
force-pushed
the
try/alternate-fixed-appender
branch
from
6f9e237 to
4a9ff26
Compare
|
Alright, I've rebased and fixed an issue with the plus not appearing in some odd cases. Specifically it wasn't working inside navigation, which I've now fixed: Shown here also the behavior that the appender for the group will only show when the group is selected, not for when content inside is selected. This is based on feedback here, (and of the 3 commits to this branch, is defined by commits 2 and 3), and is mostly important for the site editor, where without it, you'd have this:
With the change to only show the plus for the selected container, it feels much more contextual:
It essentially leans into the idea that we have too many plusses and we need to remove some of them. That also challenges a bit of muscle memory we've built up.
I tried this, and despite my previous thought agree that it looks good in many cases. However then I realized that for really small containers, like Navigation, you'd end up with this:
That's more of a critique of this button itself than anything, but it's the reason why I'm leaning towards leaving out that change for now. Here's the test content:
Here's the site editor: I wanted to bring attention to one change to what I've been referring to as the "trailing appender". This is when you have a not-text-block (such as an image) as the last block in a block list. This is the appender I think we should look at removing, separately. In trunk, it sits on the left:
In this branch it now sits on the right:
Given the plus that sits inside an empty paragraph was always on the right, I actually think this is a good change. But it's nevertheless a change. |
|
Looks like some of the test failures here are legitimate, probably due to the changes to not show the appender on ancestors. |
|
I won't press the button until you've all had a say! Take all the time you need, thank you for looking. |
|
This is very cool :) For me, the only thing that bothered me is the fact that container blocks (group, row, column, cover) don't show the appender when the last block is a paragraph, I think that behavior doesn't make sense anymore with the appender being absolute positioned. |
|
I also like all the style removals 👏 |
@ellatrix I think mentioned this particular type of appender as well. Just to be sure, do you feel like the plus should be shown regardless of content? Or shouldn't be shown? |
|
(And either way, feel free to push to this branch if you like, I think it migh be beyond my skill to change that) |
|
For me, it should always be shown (maybe just an exception for the root). Let's merge this and I can attempt this change separately. |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
|
Sounds good. I wanted to note one side effect with the navigation submenu that I'd like to follow up on separately. When clicking a submenu, the black plus is there to add a submenu item, that works well: The change to only show this black plus when the container block is selected, though, has the side effect that this plus disappears as soon as you add an item inside: I actually think this PR is healthy to merge because it shows all these side effects that are important to consider as we improve the situation here inside the component itself, with some more obvious and predictable visibility behaviors perhaps as props. In the mean time, I can whip up a quick fix for the navigation block editor.scss file to surface that black plus, even when a submenu item is selected. If you agree, feel free to press the green button and I'll follow up. |
youknowriad
changed the title
|
Just posting to confirm that the appender is now appearing as expected in Group blocks for me, and also that I, too, prefer having the appender touching the block border as it does now. |
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
Co-authored-by: Ella van Durpe <ella@vandurpe.com>
Description
Alternative to #36037, which makes the appender fixed position in the top right corner of the nesting container, and to #36602 which simply hides it entirely but only in the site editor.
This variant is based on 36037, but keeps the plus black, and positions it in the bottom right corner. The idea with this one is to make the changes feel smaller — don't redesign the inserter, still keep it towards the end of the nesting container, but just take it out of the flow so it avoids layout shifts.
Post editor demo:
Site editor demo:
How has this been tested?
Test in the post editor with this demo content, ensure no regressions:
Test in the site editor as well, with selecting nesting containers.
Checklist:
*.native.jsfiles for terms that need renaming or removal).