Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try: Make appender fixed position. #36037

Closed
wants to merge 2 commits into from
Closed

Conversation

jasmussen
Copy link
Contributor

Description

This PR is a followup to #31886, to see if it would be possible in using only CSS to take the in-canvas appender out of the flow. The key problem to solve is to prevent the current layout shift that happens when you select a block:

before

This happens because the appender shows up in the DOM and is therefore treated as if it were another block. In the case of flexed containers that space-between, that means there's another item to make room for. This PR makes that a fair bit less jarring by positioning the buttons on the focus border:

Screenshot 2021-10-28 at 13 33 34

fixed appender

That's also a bit of the challenge at the moment, as the border is present only on focus, which means as soon as focus moves inside the inserter, the button sits there, disconnected:

fixed appender

It feels like this should be solvalbe, whether through more liberal use of the is-highlighted.

An additional challenge is the abs-positioned nature of it. It looks good in the use cases shown above, but when things start to wrap on multiple lines, it's a bit less optimal:

buttons

I feel like all of this is solvable with design, but curious of your thoughts.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@github-actions
Copy link

github-actions bot commented Oct 28, 2021

Size Change: -514 B (0%)

Total Size: 1.1 MB

Filename Size Change
build/block-editor/style-rtl.css 14.5 kB +10 B (0%)
build/block-editor/style.css 14.4 kB +11 B (0%)
build/block-library/blocks/social-links/editor-rtl.css 673 B -151 B (-18%) 👏
build/block-library/blocks/social-links/editor.css 672 B -151 B (-18%) 👏
build/block-library/editor-rtl.css 9.73 kB -117 B (-1%)
build/block-library/editor.css 9.73 kB -116 B (-1%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 960 B
build/admin-manifest/index.min.js 1.1 kB
build/annotations/index.min.js 2.75 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.12 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.28 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 139 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 291 B
build/block-library/blocks/buttons/editor.css 291 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 134 B
build/block-library/blocks/code/theme.css 134 B
build/block-library/blocks/columns/editor-rtl.css 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 503 B
build/block-library/blocks/columns/style.css 502 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.19 kB
build/block-library/blocks/cover/style.css 1.19 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.62 kB
build/block-library/blocks/gallery/style.css 1.62 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 507 B
build/block-library/blocks/image/style.css 511 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 649 B
build/block-library/blocks/navigation-link/editor.css 650 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 213 B
build/block-library/blocks/navigation-submenu/style.css 213 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.89 kB
build/block-library/blocks/navigation/editor.css 1.89 kB
build/block-library/blocks/navigation/style-rtl.css 1.56 kB
build/block-library/blocks/navigation/style.css 1.55 kB
build/block-library/blocks/navigation/view.min.js 2.74 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 172 B
build/block-library/blocks/page-list/style.css 172 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 444 B
build/block-library/blocks/post-comments-form/style.css 444 B
build/block-library/blocks/post-comments/style-rtl.css 492 B
build/block-library/blocks/post-comments/style.css 493 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 396 B
build/block-library/blocks/post-featured-image/editor.css 397 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 391 B
build/block-library/blocks/post-template/style.css 392 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 245 B
build/block-library/blocks/separator/style.css 245 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 770 B
build/block-library/blocks/site-logo/editor.css 770 B
build/block-library/blocks/site-logo/style-rtl.css 165 B
build/block-library/blocks/site-logo/style.css 165 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB
build/block-library/blocks/social-links/style.css 1.32 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 569 B
build/block-library/blocks/video/editor.css 570 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 815 B
build/block-library/common.css 812 B
build/block-library/index.min.js 162 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 10.5 kB
build/block-library/style.css 10.5 kB
build/block-library/theme-rtl.css 672 B
build/block-library/theme.css 677 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.3 kB
build/components/index.min.js 214 kB
build/components/style-rtl.css 15.3 kB
build/components/style.css 15.3 kB
build/compose/index.min.js 10.9 kB
build/core-data/index.min.js 13.2 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 631 B
build/data/index.min.js 7.42 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 485 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.5 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.6 kB
build/edit-post/style-rtl.css 7.1 kB
build/edit-post/style.css 7.09 kB
build/edit-site/index.min.js 33.2 kB
build/edit-site/style-rtl.css 6.1 kB
build/edit-site/style.css 6.1 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.18 kB
build/edit-widgets/style.css 4.18 kB
build/editor/index.min.js 37.8 kB
build/editor/style-rtl.css 3.78 kB
build/editor/style.css 3.77 kB
build/element/index.min.js 3.29 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 6.57 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.63 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.71 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.8 kB
build/keycodes/index.min.js 1.39 kB
build/list-reusable-blocks/index.min.js 1.86 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 925 B
build/nux/index.min.js 2.08 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.84 kB
build/primitives/index.min.js 924 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.65 kB
build/reusable-blocks/index.min.js 2.22 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11 kB
build/server-side-render/index.min.js 1.57 kB
build/shortcode/index.min.js 1.49 kB
build/token-list/index.min.js 639 B
build/url/index.min.js 1.9 kB
build/viewport/index.min.js 1.05 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.15 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@ZebulanStanphill
Copy link
Member

With the appender appearing on the border of the container, it feels like it's intended to insert next to the container, rather than into it like it actually does.

I think that the appender should instead be attached to the last child block of the container. That would work a whole lot better in the wrapping buttons scenario, for example. Perhaps also shift the button down (for vertical flow) or to the right (for horizontal flow) to avoid overlapping the content of the block it appears after.

@jasmussen
Copy link
Contributor Author

I don't necessarily disagree, but a key part of avoiding a layout shift is the need to remove the item from the DOM. That can either be done by positioning it entirely via JS, or use position absolute which then limits our options for positioning relative to anything other than the container.

I could put it in the bottom right corner, but it remains tricky:

Screenshot 2021-10-29 at 08 20 42

@mtias
Copy link
Member

mtias commented Nov 16, 2021

Let's try the sitting on top of the line. It might not work, but we have been postponing improvements here for so long it's degrading the experience with all the layout shifts.

@ZebulanStanphill
Copy link
Member

I think the design currently used by this PR is very confusing. It looks like it inserts a sibling of the selected block (compare with the current sibling inserters that appear on hover), but it actually inserts a child. I think it would be clearer to just add a toolbar button for inserting into the current block:

image

A nice benefit of this design is that it's more obvious which block you're inserting into, since the insert button is clearly associated with the current block. Additionally, the button is also always visible, whereas currently you tend to have to scroll down to find the appender. I assume it would also be a lot less complicated to handle, CSS-wise.

With some block-specific icon tweaks, we could also make it more obvious when this button would insert a specific block (e.g. a Button into a Buttons) versus opening the library (e.g. inserting stuff into a Group).

Fixed position appenders like the ones in this PR could be repurposed for inserting siblings, which would fit their design a lot better, in my opinion.

@ZebulanStanphill ZebulanStanphill added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Nov 17, 2021
@jasmussen
Copy link
Contributor Author

Alright, I've taken a stab at refactoring this one to hopefully bring it to a state where it might land. I agree with the feedback that this needs better solutions, for example replacing it entirely in favor of the sibling inserter or possibly the block toolbar solution. And so I don't consider this PR to be the end-all solution, but rather a step on the way.

The problem is the layout shifts it's causing in the site editor are so disruptive that it's reducing the entire experience.

Here's where I've landed:

fixed appender

Take-aways:

  • The elimination of the layout shifts is really, really nice, honestly nice enough to accept a few tradeoffs as we hash this out.
  • The positioning I feel, generally works okay, especially if we see it as an interim thing.
  • The smaller size feels plenty clickable, especially since this is additive to all the other plusses.
  • But the smaller size, and the blue color, does stand out compared to the other plusses on the page. It seems like we could unify, and at least make the sibling inserter plus which is also blue, the same smaller size.

It really comes into its own in the site editor, however in doing so it surfaces a different issue:
site editor

The current behavior is for the in-canvas appender to show up both when the parent is selected, and when a child is selected. That's why in the GIF above you can sometimes see the plus seemingly "hanging there". One quick fix I can make here is to show the plus only when the actual container is selected, not when a child is selected.

Let me know what you think.

Just for regression testing, here's some test content you can use:

<!-- wp:paragraph -->
<p>Empty group, should show the big white plus:</p>
<!-- /wp:paragraph -->

<!-- wp:group -->
<div class="wp-block-group"></div>
<!-- /wp:group -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Group with content:</p>
<!-- /wp:paragraph -->

<!-- wp:group {"style":{"color":{"background":"#eaeced"}}} -->
<div class="wp-block-group has-background" style="background-color:#eaeced"><!-- wp:paragraph -->
<p>Paragraph 1</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Paragraph 2</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Group with empty paragraph inside, should show plus when selected:</p>
<!-- /wp:paragraph -->

<!-- wp:group {"style":{"color":{"background":"#eaeced"}}} -->
<div class="wp-block-group has-background" style="background-color:#eaeced"><!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Group with single image inside, should show abs positioned blue plus when selected:</p>
<!-- /wp:paragraph -->

<!-- wp:group {"style":{"color":{"background":"#eaeced"}}} -->
<div class="wp-block-group has-background" style="background-color:#eaeced"><!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div>
<!-- /wp:group -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>A few buttons:</p>
<!-- /wp:paragraph -->

<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button /--></div>
<!-- /wp:buttons -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Many buttons:</p>
<!-- /wp:paragraph -->

<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button /-->

<!-- wp:button /-->

<!-- wp:button /-->

<!-- wp:button /-->

<!-- wp:button /-->

<!-- wp:button /-->

<!-- wp:button /--></div>
<!-- /wp:buttons -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Empty cover with empty paragraph inside, paragraph should have the black plus when selected:</p>
<!-- /wp:paragraph -->

<!-- wp:cover {"overlayColor":"secondary","minHeight":262,"minHeightUnit":"px","isDark":false} -->
<div class="wp-block-cover is-light" style="min-height:262px"><span aria-hidden="true" class="has-secondary-background-color has-background-dim-100 wp-block-cover__gradient-background has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size"></p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Another cover:</p>
<!-- /wp:paragraph -->

<!-- wp:cover {"overlayColor":"primary"} -->
<div class="wp-block-cover"><span aria-hidden="true" class="has-primary-background-color has-background-dim-100 wp-block-cover__gradient-background has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size"></p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

<!-- wp:spacer {"height":50} -->
<div style="height:50px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:paragraph -->
<p>Cover with Image inside, should have the blue plus abs positioned:</p>
<!-- /wp:paragraph -->

<!-- wp:cover {"overlayColor":"secondary","minHeight":262,"minHeightUnit":"px","isDark":false} -->
<div class="wp-block-cover is-light" style="min-height:262px"><span aria-hidden="true" class="has-secondary-background-color has-background-dim-100 wp-block-cover__gradient-background has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:cover -->

but I do recommend testing primarily in the site editor.

@jasmussen jasmussen marked this pull request as ready for review November 17, 2021 12:33
@jasmussen jasmussen requested a review from mkaz as a code owner November 17, 2021 12:33
@jasmussen
Copy link
Contributor Author

@jameskoster I'd love your thoughts on this one.

@jameskoster
Copy link
Contributor

The elimination of the layout shifts is really, really nice

💯

One quick fix I can make here is to show the plus only when the actual container is selected, not when a child is selected.

I think this may be worth trying. The "hanging" inserters can really be quite awkward.

Screenshot 2021-11-17 at 12 36 10

The positioning I feel, generally works okay, especially if we see it as an interim thing.

I think it works ok on row orientations, but on columnar layouts it feels a bit misleading. Consider this example:

Screenshot 2021-11-17 at 12 40 28

My expectation here is that a block added via this inserter would appear adjacent to the selected group, which isn't the case – it appears beneath it. I'm not sure how big of a deal this really is in practical terms, but I wanted to note it regardless. Perhaps we can adjust the position based on whether the block is a row or not?

One other issue I spotted: on blocks that span the full width of the canvas, the inserter is now only accessible via horizontal scroll:

scrol.mp4

I appreciate this is an unpopular opinion, but I would be in favor of removing this inserter altogether, perhaps only in the Site Editor to begin with, but probably in the post editor too :)

There are many ways to insert blocks, and I'm just not seeing the value of this one based on the annoying layout shifts, and difficulties in circumventing that.

The sibling inserter allows you to inject blocks between others. The primary inserter in the top bar will insert the block in to the selected container. What purpose does the appender provide that is not covered by those other two inserters?

@jasmussen
Copy link
Contributor Author

jasmussen commented Nov 17, 2021

Great thoughts, thank you for looking.

My expectation here is that a block added via this inserter would appear adjacent to the selected group, which isn't the case – it appears beneath it. I'm not sure how big of a deal this really is in practical terms, but I wanted to note it regardless. Perhaps we can adjust the position based on whether the block is a row or not?

There's really no way for me to tell using just CSS, so that's not really something I can do as a small interim iteration. And perhaps that's worth reiterating: this PR is an experiment to see if we can ship 5.9 without layout shifts on select. I would hope we have something better for 6.0. That's also a good way to frame the feedback: what fixes can we make to land this for 5.9?

I appreciate this is an unpopular opinion, but I would be in favor of removing this inserter altogether, perhaps only in the Site Editor to begin with, but probably in the post editor too :)

Oh I am too! And I do think the sibling inserter, especially if we address #35536, can replace this one and be a good experience.

What purpose does the appender provide that is not covered by those other two inserters?

The sibling inserter lets us do this:

inbetween

But it doesn't allow us to do either of these; insert after the last block in a container:

after

after-block

Insert before the first block in a container:

before-block

Notably for the "Buttons" use case, it really seems like there needs to at least be an "insert after the last block in a container" behavior in place [horizontally], no?


Depending on additional feedback, the todo-items (note to self):

  • Fix issues with full wide container blocks
  • Hide the appender on the parent

@jameskoster
Copy link
Contributor

Notably for the "Buttons" use case, it really seems like there needs to at least be an "insert after the last block in a container" behavior in place [horizontally], no?

What if the sibling inserter was updated to also appear before the first block in a container, and after the last block? 🤔

@jasmussen
Copy link
Contributor Author

Yep, I'm saying I think that would work. I just also assume that's non trivial to do.

@priethor priethor added the [Priority] High Used to indicate top priority items that need quick attention label Nov 17, 2021
@jasmussen
Copy link
Contributor Author

Alright, for folks subscribed to this one, here's a small status update.

The problem to solve remains to avoid the layout shifts that happen when you select a nesting container and the "Plus" button appears. It's important to keep this in mind, because the experience that's shipping is definitely compromised by those shifts, especially in the site editor, and it's the primary reason why we should consider an interim solution to the problem, and then come back with a better long term solution to replace it.

I have three different PRs that aim to solve this:

1. What you see in this branch (36037). It affixes the plus to the top right of the container, makes it small and blue.

2. #36602. This one entirely hides the plus, but only in the site editor.

3. #36605. Based on 36037, but doesn't visually change the plus, and places it in the bottom right corner of the container.


Of the three, I personally prefer 2 or 3 as they feel like smaller changes. Let me know your thoughts.

@jameskoster
Copy link
Contributor

Nice! For 3 did you try making the appender only appear for the closest container? I noticed that when you select the buttons block, the appender for the containing column still popped up in addition to the one for the buttons block. I'm also wondering how the [+] interacts with right-justified content, the overlap seems like it could be a little awkward?

Overall I like 2 and 3 the most as well, with a slight preference for 2 as I feel we've over-indexed on inserters and the appender seems to be the least useful on balance.

@jasmussen
Copy link
Contributor Author

I tried that just now, and it's easy enough to get to work for the inner containers:
direct ancestors

However it appears there's an issue attached to that which relates to this same component being used for an empty group block which shows the white plus. I'll see if I can get it working there as well.

@jasmussen
Copy link
Contributor Author

@jameskoster okay I have a fix working that I can push to #36605 if we like it. It works like the GIF I just shared, but it does come with this downside:
button

That is, you now have to select the exact container in order to get the inserter to appear. In the case of the buttons block, that means the inserter disappears when you select a button (singular). We could consider that a boon, but wanted to just sanity check before I push it. What do you think?

@jameskoster
Copy link
Contributor

I'm inclined to say that's probably ok. Once again it feels like the lesser of two evils.

But ultimately I feel this just adds more weight to the idea of getting rid of it altogether. There are just so many quirks and compromises around this thing 😅

@jasmussen
Copy link
Contributor Author

Thanks, I pushed the fix.

But ultimately I feel this just adds more weight to the idea of getting rid of it altogether. There are just so many quirks and compromises around this thing 😅

Actually I think the fact that we are seriously looking at these temporary fixes acknowledges how bad the situation is, and that it needs a developer. That isn't me advocating for one solution over the other (though 3 is growing on me), just that I have full confidence this will finally get the attention it needs.

@jasmussen
Copy link
Contributor Author

Closing in favor of #36605.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants