I like the direction this goes! It's a plus one from me, because I think showing multiple block inserters at the same level is confusing in this situation. Particularly because the two appearing close together (from the group block and from the template part block) don't seem like they would insert a block into separate entities, but they actually do.

Size Change: +51 B (0%)

Total Size: 1.17 MB

_{compressed-size-action}

Note:

I noticed interesting behavior, which seems intended, while testing this fix. The trailing inserter is only shown if the last block in a set of inner blocks is not a paragraph. If you're not seeing the trailing inserter when you'd expect it to display, add any block besides a paragraph block to the last position of a group block, query loop block, etc.

If you're curious about technical details, the execution thread leads here to these lines of code, which simply check if the name of the last block is core/paragraph. If it returns true, the trailing inserter is not rendered.

The special-case behavior for Paragraph blocks definitely was intended at some point, but I suspect time may have changed a lot of people's opinions on it. (I recall how lots of people were on board for increasing block padding for parent blocks to make them easier to select... only for a lot of the same people to change their minds a few months later after better solutions were found and implemented. Similarly, I recall how the decision to remove the quick-insert shortcuts was almost comically unanimous.)

Personally, I've come to think the no-appender behavior with Paragraph blocks is weird and inconsistent, since even other text-related blocks lack the behavior. I think we ought to make the behavior consistent across all blocks.

Anyway, as for this PR, I like the idea behind it. I think it's a definite improvement over master, keeping in the spirit of only showing controls contextually and looking like the front-end as much as possible.

I did catch one issue, though. Check out empty Column blocks:

I did catch one issue, though. Check out empty Column blocks:

Ah, yes im seeing this as well. Actually inserting the columns block is a bit uglier on my end, it ends up removing that block appender and the selected column has no height:

Where normally we would expect:

So this seems to be effecting more than just the sibling appender 🤔

@Addison-Stavlo This PR was never modifying the sibling inserter in the first place. It's modifying the "trailing appender" (or whatever it's called). As it turns out, the Column block placeholders are just restyled trailing appenders, hence why they're affected.

Thanks very much for tackling this head on! The issue with multiple inserters is especially frustrating in patterns where you might see a group with a background, a columns inside, then an image, and you'll end up with 3 black plus buttons. This is an important problem to solve, as in many cases it actuallly breaks the layout and definitely visually misleads.

The precise behavior is hard to get right, though. The primary challenge the trailing appender is meant to solve is the situation where a non-text block is the last block in a container, such as an image. While you can always select an image and press Enter when it has focus and it creates a new paragraph beneath, this in practice has not been intuitive to folks. Importantly, that issue appears to be addressed in this PR, and generally I would focus the most testing on ensuring that aspect is handled well. For that case, the principle "only show one at a time" seems right.

In superficial testing, the heuristic doesn't seem perfect yet for an empty post. This seems a bit bare:

@jasmussen @ZebulanStanphill @Addison-Stavlo

Thanks for the feedback! The columns block and empty post state should be fixed.

An important change to note is that, with the columns block, only one trailing inserter is displayed at any given time. Given the modifications we're making in this PR, this is expected, but it definitely changes the columns block experience. I'm not sure if anyone feels strongly about UX updates.

Before

After

Nice updates, I like how this is taking shape. However, I am noticing another side effect here. Previously, when we had the Column itself selected we were given the appender at the bottom of that column. Now with the column selected there doesn't seem to be an appender for it, we must select a block in that column in order to get the appender to appear:

Before

After

But still appears when the block in the column is selected:

Regarding the 3 trailing appenders (one for each column) - This is actually behaving oddly for me on master. I reproduced it with the 3 appender once, but am unable to reproduce it anymore. A lot of the time I am just seeing the appender for just the one column that is selected anyways. Even chalking this up to my own local env weirdness, and assuming that we always currently have the 3 appenders on master, I feel only showing the appender for the selected column is not a bad change.

However, I am noticing another side effect here. Previously, when we had the Column itself selected we were given the appender at the bottom of that column. Now with the column selected there doesn't seem to be an appender for it, we must select a block in that column in order to get the appender to appear

@Addison-Stavlo This is a good observation 👍. I wanted to note that this behavior is expected in the sense that, in our code, we've outlined logic to only display the trailing inserter for one set of inner blocks at any given time.

To elaborate, clicking on an inner block for one column, like a Site Logo block, displays the trailing inserter for column inner blocks. Clicking on an inner block for column(s), however, like a column block, should display a different trailing inserter. It's empty because there is no trailing inserter for columns inner blocks, which is also behavior that exists on master.

Click on column inner block	Clicking on columns inner block

With all of that being said, I do agree that the "Before" / previous behavior seems more ideal for user interactions. I personally expect to see a trailing inserter for column inner blocks, regardless of whether or not I click on the column block or a column inner block. I only wanted to call this out because the change to address this would be more of an exception to our new business logic for trailing inserters, and less of a "fix" for aberrant behavior.

Hopefully that makes sense. I'm leaning towards implementing the code to create an exception for column blocks, but let me know if you think otherwise.

Hopefully that makes sense. I'm leaning towards implementing the code to create an exception for column blocks, but let me know if you think otherwise.

Are there other blocks to which this could apply, or maybe a more general way to improve this? I guess, what exactly makes the columns block special in this situation?

Hopefully that makes sense.

Yeah, that makes sense!

I'm leaning towards implementing the code to create an exception for column blocks, but let me know if you think otherwise.

It seems to be a change to group blocks as well. Previously with the group block selected the appender was added to the bottom inside the group block, similarly now it is not. Im not sure what the best approach is here, but creating case by case exceptions may not be the best way to go about it. 🤔 Maybe the extra qualifier should be that if it is a block that can contain inner blocks it will still need the appender inside of it? May need more external input here...

It seems to be a change to group blocks as well. Previously with the group block selected the appender was added to the bottom inside the group block, similarly now it is not.

Oh good catch. This is going to happen with any block that implements inner blocks. An inserter should still be displayed alongside the group block, but not for its content. @Addison-Stavlo

Maybe the extra qualifier should be that if it is a block that can contain inner blocks it will still need the appender inside of it?

This would reintroduce the look of "duplicated" inserters. Imagine a nesting like group 1 > group 2 > paragraph. Group 1 and Group 2 can both have inner blocks. With that logic, an inserter to place blocks in group 1 will always be displayed alongside the group 2 block because it will have the extra qualifier. Another inserter to place blocks in group 2 will be displayed alongside the paragraph block because it will also have the extra qualifier.

maybe a more general way to improve this?

I'm going to give the business logic more thought. My intuition says that it might not be possible with our current approach.

@Addison-Stavlo I pushed changes to support the use case where we want the inserter block displayed when a parent is selected. It should address the issues you brought up with the group and column blocks.

It reintroduces some instances where we see "duplicate" inserters, specifically when we select a block with inner blocks nested within another block. (Ex. Group 1 > Group 2 > Paragraph, where we select Group 2) I still think that overall, it's an improvement over the behavior we were struggling with before.

Everything looks to be in a good place. This PR is ready for another round of CR and feedback @ZebulanStanphill @MichaelArestad @noahtallen @Addison-Stavlo 🙂

Thanks for the help everyone!

I'm not exactly sure which edge cases to test for here, so I decided to give it a spin and see how it felt. Unfortunately, I don't think this fixes the use cases I was expecting it to fix!

This is on a fresh local install. I've made no edits: just selected to the left of where the top template part is so that it would select the blocks there.

I think the block structure in this theme is:

- group
  - template part
    - group (this is the one selected in the screenshot above

I'm not exactly sure which edge cases to test for here, so I decided to give it a spin and see how it felt.

@noahtallen I can clarify what we can expect from this PR now:

When we select blocks with no inner blocks, there should no longer be duplicate inserters. (Ex. Selecting Paragraph from Group > Template Part > Paragraph)

2. When there are sets of nested inner blocks and we select the outermost parent, there should no longer be duplicate inserters (Ex. Selecting Group from Group > Template Part > Paragraph)

3. A trailing inserter for the root document will always be shown as it is today.

Seeing duplicated inserters when selecting a block (with inner blocks), nested in another set of inner blocks, was previously discussed here and here. There are certain blocks (like the column and group block) where we'd like to display the trailing inserter when:

1. The block itself is selected
2. The block's children are selected

This leads to the issue you're seeing. When we have a series of nested inner blocks, we run into the situation where we display 1 inserter for the selected block (because it's selected # 1) and 1 inserter for its parent (because the parent's child is selected # 2). It's an unfortunate tradeoff, and if this doesn't quite improve the template part editing experience enough, maybe we could explore your suggestion here @noahtallen ?

I'm not super happy with it here, but I can definitely accept that it is necessary for supporting other blocks. I do wish we could be a bit smarter about it.

It definitely felt like we could be smarter about it. I spent some time last night thinking about a few last changes we could make to improve the experience. Long story short, I add a condition that checks for nested inner blocks. It should fix the issues you were seeing earlier, Noah. Not sure if it's perfect, but it's an easy revert, and I think it noticeably improves the editing experience. Let me know what you think @noahtallen @Addison-Stavlo

Hm. I think we might want to play it safe and not include that change for now. It is more difficult to add blocks in scenarios where you have column -> group. You can't add another block to the column unless you select the column itself in this scenario, which is very difficult if you aren't comfortable with "select parent block" or the breadcrumb.

But I do really like it for the template part use cases we are looking at :D