Improve block listing in Layout Builder by hiding uncommon block plugins

Contrib has already started on this: Layout Builder Restrictions. I think it makes sense for core to provide this to avoid the jarring out of the box experience that currently exists with the overwhelming list. If core absorbed that work, it could provide a default set of blocks that are hidden/not shown while still allowing edge cases to use them if they want.

First a +1

I think this definitely should be default behaviour, this is a great UX improvement and is especially great for the new Drupalisters when trying LB.

However, it's important that these can be re-added easily which is why I think a UI would probably be a good option to do this and ideally enabled by default.

If we're not keen to add a UI because reasons, the original definitions should be added to the hook so you can, again add them back easily.

I haven't looked at Layout Builder Restrictions yet but think core do a lot just promoted the entity fields where $field_definition->isDisplayConfigurable('view')

This patch does
Changes FieldBlockDeriver to create 2 categories for each entity type 1 for 'view' configurable field and one for those that aren't. For example for nodes it creates "Content" and "Content(extended)".
Changes ChooseBlockController to close all categories except Field Blocks for "view" configurable fields. Moves all field block categories to the top.

This will produce something like this for nodes:

1. I talked to @DyanneNova who suggested that we not a new category but instead nest the additional field with a "More+" link that shows them.

   Here is try at that except using a nested details element. Right it still looks like it on the same level as the other categories but think some CSS it would work.

   

   No doing an interdiff b/c these 2 approaches are pretty different.

2. While working on this realized that not everything under a category "[ENTITY_LABEL]" like "User" or "Content" is going to be a field which could get pretty confusing since the blocks could easily have the same labels as fields.

   So I created #2978939: Change 'field_block' block plugins to use "[entity_type] - fields" category to avoid confusion with other blocks

Added test. While working on the test I realized there would a case where an entity type has only non 'view' configurable fields. This was the case for the test for the 'User' category. I think if you removed the "Picture" field after Standard profile install that would be the case.

🎉 for writing tests so we find this stuff out.

When this is the case it doesn't make sense create the "More+" section as there would be no other links in the category.

So functionality and test coverage is:

1. If a entity type has 'view' configurable fields( or other links) that show in the category it should be moved to the top, opened by default and non 'view' configurable fields should be in a closed sub "More+" section.
2. If a entity type has no 'view' configurable fields( or other links) that show in the category it should not be moved to the top and it should be closed with no "More+" section.
3. All non entity sections should be closed by default.

The reason a "entity" category could have no 'view' configurable fields but still have link directly under the category is because of #2978939: Change 'field_block' block plugins to use "[entity_type] - fields" category to avoid confusion with other blocks

🎉 for writing tests so we find this stuff out.

🎉for actually uploading the files 😜

+++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
@@ -67,13 +82,16 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
     foreach ($this->blockManager->getGroupedDefinitions($definitions) as $category => $blocks) {

This bit is stale, it's currently

    $definitions = $this->blockManager->getFilteredDefinitions('layout_builder', $this->getAvailableContexts($section_storage), [
      'section_storage' => $section_storage,
      'region' => $region,
    ]);

Could this patch be made into a part of the filtering code? Or are there changes 100% needed in this markup?

1.

This bit is stale, it's currently

Not sure what you referring to the code that you mention with the call to getFilteredDefinitions() is already in the patch about this.

We still need to call \Drupal\Component\Plugin\CategorizingPluginManagerInterface::getGroupedDefinitions() we need to call this to get the definitions in sorted groups with the labels also sort by definitions. Because we are sending the $definitions it will use our filtered list.

2.

Could this patch be made into a part of the filtering code? Or are there changes 100% needed in this markup?

I don't think we can do this in filtering. We aren't actually filtering out any plugins. Just nesting non view configurable fields(unless they are the only ones for entity)

Rerolled

:(

This patch causes an error in 8.6-rc2:

Undefined variable: field_block_category_weight in /var/www/docroot/core/modules/layout_builder/src/Controller/ChooseBlockController.php on line 148↵Notice: Undefined variable: field_block_category_weight in /var/www/docroot/core/modules/layout_builder/src/Controller/ChooseBlockController.php on line 149↵

when trying to add a new block. Looking at the code, this patch adds:

      // If this a entity category and there are links besides non 'view'
      // configurable field blocks move the category to the top and open it.
      if (in_array($category, $entity_type_labels) && $links) {
        $build[$category]['#open'] = TRUE;
        $build[$category]['#weight'] = $field_block_category_weight;
        $field_block_category_weight += 10;
      }

in the ChooseBlockController and the $field_block_category_weight is not defined anywhere.

That got lost in the reroll, it was in #11

Fixing invalid use statement in FieldBlockDeriver.php.

Fixing tests that were failing and some that would fail now that #2978939: Change 'field_block' block plugins to use "[entity_type] - fields" category to avoid confusion with other blocks is in.

Created a base test because other tests will now need to click categories before adding some blocks.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
@@ -0,0 +1,208 @@
+      if ($details->find('css', "summary:contains('$category')")) {

i don't think we can use :contains anymore because really we want an exact match on the category. Fixing

Screenshots
First opened

After clicking "More" under content fields.

Whoops left debug statement in.

Nice! I found some stuff, but nothing serious. Overall I think this makes a lot of sense.

1. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -60,6 +74,10 @@ public static function create(ContainerInterface $container) {
   +    foreach ($entity_type_labels as &$entity_type_label) {
   

   Why is $entity_type_label a reference?

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -72,13 +90,16 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +      $non_view_configurable_field_links = [];
   +      $links = [];
   

   I'm not sure how I feel about these variable names; I don't think they clearly communicate the intent. Maybe $main_field_block_links and $secondary_field_block_links would be better?

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -97,7 +118,38 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +            '#title' => $this->t('More+'),
   

   I feel like we should add the + using CSS, rather than stylizing the string here.

4. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -97,7 +118,38 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +      if (in_array($category, $entity_type_labels) && $links) {
   

   in_array() should have TRUE as a third argument.

5. +++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
   @@ -118,6 +118,11 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +          // Mark as view configurable or not to enable highlighting of view
   +          // configurable fields in the 'Add Block' listing of the layout
   +          // builder.
   +          $derivative['_is_view_configurable'] = $field_definition->isDisplayConfigurable('view');
   

   Can we add a @see reference here?

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestBase.php
   @@ -0,0 +1,34 @@
   +    $page = $this->getSession()->getPage();
   +    $page->find('css', "#drupal-off-canvas summary:contains('$category')")->click();
   

   Pro-tip: If find() cannot find the link, it will return NULL, which will cause a fatal if you try to call ->click() on it. A more preferable way to do this would be:

   $this->assertSession()->elementExists('css', '#drupal-off-canvas summary:contains('$category')")->click();
   

   This will fail gracefully (i.e., an assertion, error message, proper cleanup) if the link does not exist.

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestBase.php
   @@ -0,0 +1,34 @@
   +   * Asserts that block link is not visible.
   

   The method is called assertBlockLinkVisible(), and the assertion is checking that the link is, in fact, visible. :)

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestBase.php
   @@ -0,0 +1,34 @@
   +    $link = $this->getSession()->getPage()->find('css', "#drupal-off-canvas a:contains('$link_text')");
   +    $this->assertNotEmpty($link->isVisible());
   

   Same idea here -- use assertSession() to be sure the element exists, then you can call isVisible() on it. Also, why use assertNotEmpty() here instead of assertTrue()?

   Alternately, wouldn't $this->assertSession()->waitForElementVisible() work here?

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,193 @@
   +  public static $modules = [
   +    'layout_builder',
   +    'block',
   +    'node',
   +  ];
   

   Should be protected.

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -0,0 +1,193 @@
    +    $assert_session->linkExists('Manage layout');
    +    $this->clickLink('Manage layout');
    

    Pro-tip: You can do this in one line:

    $assert_session->elementExists('named', ['link', 'Manage layout'])->click();

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -0,0 +1,193 @@
    +  /**
    +   * Asserts that block link is visible.
    +   *
    +   * @param string $link_text
    +   *   The link text.
    +   */
    +  protected function assertBlockLinkNotVisible($link_text) {
    

    Doc comment is wrong. Also, shouldn't this method be in LayoutBuilderTestBase?

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -0,0 +1,193 @@
    +    $this->assertEmpty($link->isVisible());
    

    Why not use assertFalse() here?

13. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -0,0 +1,193 @@
    +      if ($summary instanceof NodeElement && $summary->getText() == $category) {
    

    Should be ===

14. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -0,0 +1,193 @@
    +    $this->assertEquals($categories, array_values(array_intersect($display_categories, $categories)));
    

    I wonder if assertSame() is more correct here.

15. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -0,0 +1,193 @@
    +    return $this->findCategoryDetails($category)->find('css', "summary:contains('More+')");
    

    If the details element is not found, ->find() will be called on null and the test will fatal. Maybe this should assert that the details is an instance of NodeElement before it calls ->find() on it?

@phenaproxima thanks for the detailed review! 🙇‍♂️
1. Because we are assigning a new value to it in the loop.

Actually this is kind of confusing I am just going to create a new array $entity_field_categories because that is more accurate.
And get rid of need for $entity_type_labels
2. $links will be all block links except field blocks links for field blocks where the field is not view configurable not just field blocks links. So all other blocks links would be in there 2. This because loop with go through all categories.
I change to $secondary_field_block_links and $block_links
3. fixed
4. fixed
5. added
6. fixed, thanks for the tip
7. fixed
8. Changed to
$this->assertNotEmpty($this->assertSession()->waitForElementVisible('css', "#drupal-off-canvas a:contains('$link_text')"));
Because waitForElementVisible won't actually fail the test because it is not an assertion.
9. fixed
10. I feel like doesn't read as easily so the test is not clear. Looking for "named" element? Leaving
11. Fix doc. Moved to LayoutBuilderTestBase
Also change to
$this->assertEmpty($this->assertSession()->waitForElementVisible('css', "#drupal-off-canvas a:contains('$link_text')"));
to match the new logic in assertBlockLinkVisible
12. See 11 change
13. fixed
14. fixed
15. fixed

Sorry had to re-roll not interdiff

Whoops forgot to add a couple of files

+++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
@@ -118,6 +118,12 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+          // @see \Drupal\layout_builder\Controller\ChooseBlockController::build()s
+          $derivative['_is_view_configurable'] = $field_definition->isDisplayConfigurable('view');

There's a stray 's' at the end of the @see line. :)

Patch tested, and plays really nice, I really like it.

Only some minors.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +   * Clicks the 'More +' element of a category.
   +   *
   

   Remove '+' :)

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +   *   The category to find.
   ...
   +    $all_details = $this->getSession()->getPage()->findAll('css', "#drupal-off-canvas .block-categories details");
   

   I'd say that we don't need double quotations for the CSS selector here.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +    $all_details = $this->getSession()->getPage()->findAll('css', "#drupal-off-canvas .block-categories details");
   ...
   +    foreach ($page->findAll('css', "#drupal-off-canvas summary") as $category_summary) {
   

   Prefer single quotations.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +    return $details_element->find('css', "summary:contains('More')");
   

   Prefer single quotation mark.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +    // "More" link and the category will be closed by default.
   

   This is the only row where double quotation marks are used.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +      if ($summary instanceof NodeElement && $summary->getText() === $category) {
   

   It's enough to check that $summary is not NULL, no need for instanceof.
   if ($summary && $summary->getText() === $category)

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -0,0 +1,184 @@
   +    $this->assertInstanceOf(NodeElement::class, $details_element);
   

   $this->findCategoryDetails will return NodeElement or NULL. Why not use assertNotNull here?

Edit: after discussing with @phenaproxima, #6 and #7 MUST be ingored. The `instanceof` thing is good — it helps IDEs.

@huzooka thanks for the review, good catches.
1. Fixed
2. fixed
3. fixed
4. fixed
5. fixed

Also fixed #35

Code looks good to me, and @huzooka has confirmed the patch works. We're also using it in Lightning. I'd say this is good to go.

+++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
@@ -75,13 +91,16 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
+      $secondary_field_block_links = [];
+      $block_links = [];
       foreach ($blocks as $block_id => $block) {

Sorry to knock back from RTBC, but I didn't get a chance to jump in here until now.

Could this loop be turned into a protected method that is passed $blocks and $build[$category] or similar, and move all of this out of the (already too big) build method?
I think it would go a long way to keeping the code readable.

Refactored getCategoryBuild()

And also refactored isFieldCategory() to determine if a category is a field block category.

Beautiful

This one needs a re-roll since the test class name conflicts with the one from #2919795: Add visual hints that Layout Builder work is in tempstore and will not be lost, or take effect until saved.

The apparent "after" screenshot in #28 looks vastly improved... I guess one concern tho is there are two primary cases for Layout Builder, at least that I can think of off the top of my head. The first is you're customizing the way an entity type looks overall and/or how a specific entity will look (node/user), and these defaults seem to work great for that. But the second is when you're creating a landing page/marketing promo page/etc, in which case you mainly want to bring in usually Views or custom one-off blocks you've made for said page. Those options now get buried in it looks like nine (minimum; since this is just core) other possible collapsible headings to click on. (And they all have very "Drupally" names :\ I realize that's out of scope, tho.)

Could more info be provided on the design rationale here for the hide/show logic? When people have seen this used in the field, are the default options exposed here enough for the "80%" case, or do we need to do some more design work? (Not necessarily in this issue—this is a huge step forward already.)

Rerolled as both this issue and another recently committed issue add the LayoutBuilderUiTest class.
Switched from a base class to a trait.

As this no longer removes plugins like "Main Content Block", I think the IS should be updated.

Renamed the name in the file, but not the name *of* the file. Whoops.

I believe the UX and UI scalability impact of this qualifies it as major.

@webchick thanks for the feedback

But the second is when you're creating a landing page/marketing promo page/etc, in which case you mainly want to bring in usually Views or custom one-off blocks you've made for said page.

This is a good point.

there are a couple of related issues that will make this situation better though.

1. #2968500: Change inline blocks workflow in Layout Builder to match mocks
   Will add a "Create new Content" or "Create new block"(wording to be decided) at the top of the block list.
   
   This will either take you directly to creating a custom block or to a list of custom block types, if you have more than 1
   This will make the "custom one-off blocks " easier to find
2. #2998862: The Layout Builder block listing should be filterable
   This will give a text box to filter/search for blocks. So hopefully it won't matter as much that blocks are "hidden" inside categories. The blocks will become visible if they match the search.
   
3. One other thing we could do on this issue to make the landing page situation better.

   If we imagine the site builders will set up a new content type for landing pages that won't actually contain addition fields, because they will use custom blocks, view, etc, then we could actually have the listing behave differently in the case that the entity using the Layout Builder has no view configurable fields.

   Right now we expand out any category that contains view configurable fields. So for the case of a node in the Layout Builder that will mean the node itself, if the bundle has view configurable, and the user, because the standard profile has a picture field.

   So that means that even for a landing page type with no View configurable fields on the bundle the User Fields category would still be expanded if the profile image was there.

   We could change the behavior of this patch to only expand out only the X fields category for the current entity type. So when using the Layout Builder for a node only expand out Content Fields and only if there are view configurable fields.

   This would mean that for a landing page content type(or other content entity) with no view configurable fields we would have no category to be expanded by default. In that case maybe it would make sense to expand out all categories. So when making a landing page everything would exposed by default.

Can we add a before screenshot to the IS, and the latest after screenshots? I don't see the "before" on the issue at all.

The two issues in #47 could be listed in the "remaining tasks" section.

Thanks!

Marking for screenshots and reroll.

Rerolled, and added some before/after screenshots to the IS.

Adding the two issues mentioned in #47 to the IS, and to the related issues.

This is very clearly a usability concern, so tagging accordingly.

1. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -76,34 +88,107 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +    static $field_block_category_weight = -200;
   

   Feels weird to use static keyword inline instead of a static class property. Any particular reason?

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -76,34 +88,107 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +      if ($block['id'] === 'field_block' && empty($block['_is_view_configurable'])) {
   

   Weird to check both the is_view_configurable flag AND the ID (also how is that the ID, not field_block:something:something:whatever?

   I'd think it should be completely determined by a flag on the definition, so others could opt-in if they chose.

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -76,34 +88,107 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +        $entity_field_categories[] = $this->t('@entity fields', ['@entity' => $entity_type_label])->render();
   ...
   +    return in_array($category, $entity_field_categories, TRUE);
   

   Wow, super odd to do this comparison via t() (and render()!?)
   Are we sure this works when things are translated?

4. +++ b/core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
   @@ -118,6 +118,12 @@ public function getDerivativeDefinitions($base_plugin_definition) {
              $derivative['_block_ui_hidden'] = !$field_definition->isDisplayConfigurable('view');
   ...
   +          $derivative['_is_view_configurable'] = $field_definition->isDisplayConfigurable('view');
   

   This is a bit odd, no?

@xjm, @phenaproxima and @tim.plunkett thanks for pushing this issue forward.

Re #53

1. It felt weird to make it class level when it should only ever be accessed in the method. Prevents accidently changing the value elsewhere. Or having to document it when it only applies to the methods logic.

2. also how is that the ID, not field_block:something:something:whatever?

   For the blocks that use derivers the ids don't change. I checked this other block type and all views block definitions the id is views_block

   I'd think it should be completely determined by a flag on the definition, so others could opt-in if they chose.

   I changed this to _is_secondary_layout_builder_block
   How do document this? layout_builder.api.php?

3. OK this was because and object would be returned so in_array wasn't matching.

   changed to
   $entity_field_categories[] = (string) $this->t('@entity fields', ['@entity' => $entity_type_label]);
   Which will get the translated string.

4. saving to $field_definition->isDisplayConfigurable('view'); to a variable first

Since #2968500: Change inline blocks workflow in Layout Builder to match mocks patch #54 no longer applies.

My attempt of a reroll.

Fixed the tests.

@bendeguz.csirmaz thanks for the re-roll

+++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
@@ -188,13 +310,19 @@ public function inlineBlockList(SectionStorageInterface $section_storage, $delta
+   * @param callable $predicate
+   *   (optional) The filter criteria applied to blocks.
    *
    * @return array
    *   The block links render array.
    */
-  protected function getBlockLinks(SectionStorageInterface $section_storage, $delta, $region, array $blocks) {
+  protected function getBlockLinks(SectionStorageInterface $section_storage, $delta, $region, array $blocks, callable $predicate = NULL) {

This logic the callable $predicate seems overly complex to me.

If seems if getBlockLinks() just had argument $block_level that had 2 possible values, "primary" or "secondary" then you could get rid of isPrimaryBlock() and isSecondaryBlock().

And then possibly just call getBlockLinks() directly and then you could get rid of getPrimaryBlockLinks() and getSecondaryBlockLinks().

At the very least $predicate should not be an optional parameter. We are only calling it twice. But would be in favor of just using something like $block_level.

Yes, I agree it's somewhat complex (I was thinking of creating a new class for this).

The point of making that parameter optional was that this function is called in existing code.
So if we go with the flag approach, we need at least 3 flags ("unfiltered", "primary" and "secondary").

I think a better approach is to just filter the blocks before passing them to the getBlockLinks function.

I was thinking something like this.

1. The point of making that parameter optional was that this function is called in existing code.

   Whoops, I missed the third call with out the parameter.

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -164,17 +164,25 @@ protected function getCategoryBuild(SectionStorageInterface $section_storage, $d
   +    $is_primary_block = function (array $block) {
   +      return empty($block['_is_secondary_layout_builder_block']);
   +    };
   +    $is_secondary_block = function (array $block) use ($is_primary_block) {
   +      return !$is_primary_block($block);
   +    };
   +    $primary_blocks = array_filter($blocks, $is_primary_block);
   +    $secondary_blocks = array_filter($blocks, $is_secondary_block);
   

   Yeah I like the filtering before sending to getBlockLinks()

3. We still need to document _is_secondary_layout_builder_block I guess in layout_builder.api.php

Here's a patch with styling for the "More" subsections. Visually it's not quite popping for me - There are additional changes I'd make if this styling was specific to Layout Builder. Since these styles are applied to the off-canvas in the Stable theme, I only made changes to nested <details>so ensure it would not change the appearance of off-canvas content anywhere else on the site. Still accomplishes the necessary visual distinction.

Screenshots provided showing open, closed with category-beneath open, and closed with category-beneath closed.

1. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -124,15 +136,95 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +  protected function getCategoryBuild(SectionStorageInterface $section_storage, $delta, $region, array $blocks, $category) {
   

   Can we rename this method to something more "active"? How about buildCategory()?

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -124,15 +136,95 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +    // categories can be moved ot the top.
   

   "moved to"

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -124,15 +136,95 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +    $is_secondary_block = function (array $block) use ($is_primary_block) {
   +      return !$is_primary_block($block);
   +    };
   +    $primary_blocks = array_filter($blocks, $is_primary_block);
   +    $secondary_blocks = array_filter($blocks, $is_secondary_block);
   

   $is_secondary_block seems a bit superfluous. Once we've filtered the primary blocks, couldn't we just use array_diff_key() or something to get the secondary blocks?

4. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -124,15 +136,95 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +        $category_build['more_fields'] = [
   

   Should the key maybe be more_blocks, rather than more_fields?

Re #64

I have te

+++ b/core/themes/stable/css/core/dialog/off-canvas.base.css
@@ -37,6 +35,10 @@
+#drupal-off-canvas details details summary {
+  background-color: #444;
+  font-weight: normal;
+}
 #drupal-off-canvas h1,

I don't think we should make changes to "stable" them in this issue.

Can we keep the changes to layout Builder css with selectors to make sure it only effect our dialog callback.

Or if the current patch is functional then would just say don't include the css in this issue and open general issue for off-canvas css.

If the CSS changes are necessary for this issue then we should keep inside layout builder.

Removing the "array api"

I talked with @tim.plunkett about his suggestion in #53.2

I'd think it should be completely determined by a flag on the definition, so others could opt-in if they chose.

We agree that:
this is making an "array as API" which we don't want.

1. The LB ui is not an API
2. for entities the most important items are actually fields.
3. Among these field the most important ones are the display configurable fields
4. Layout Builder module is free to make that decision and not provide an api for promoting other items.
5. if another module really wants something to be promoted like fields there are several of ways to make it is field regular field, computed field or extra field

So this patch:

1. Removes the _is_secondary_layout_builder_block logic
2. Moves only non-view configurable fields under "More"
3. Changes the category of Extra Field blocks to "[Entity Type] Fields". This was an oversight I think in #2978939: Change 'field_block' block plugins to use "[entity_type] - fields" category to avoid confusion with other blocks. And for the purposes of this issue when want show by default what the user would consider view configurable fields. Since extra blocks show up just as regular fields and the only reason you would create a 'display' extra field is if you want the user to be able to configure it's display.

Review #65

@phenaproxima thanks for the review

1. fixed
2. fixed
3. Good call. This part has change but we can get rid of that function in similar manner.
4. I think you were correct but now that logic only deals with field block and extra field blocks I think it fits now.(I promise I didn't change the patch just to make this key fit 😜)

Other changes

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestTrait.php
   @@ -0,0 +1,40 @@
   +    $this->assertNotEmpty($this->assertSession()->waitForElementVisible('css', "#drupal-off-canvas a:contains('$link_text')"));
   ...
   +    $this->assertEmpty($this->assertSession()->waitForElementVisible('css', "#drupal-off-canvas a:contains('$link_text')"));
   

   I realized we actually don't need the wait here because aren't waiting for ajax request. We are just clicking html details element which doesn't involve css.

   This makes the test run a lot faster because assertBlockLinkNotVisible() would always take the full wait time before.

   I removed this from trait because is less generic after removing the wait when you might want to check if a block link is visible and that would include a wait for the list to appear.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -92,4 +97,153 @@ protected function assertModifiedLayout($path) {
   +    // The 'User fields' category should not be moved to the top if there are no
   +    // 'view' configurable fields.
   +    $this->assertRelativeCategoryOrder([
   

   Now that the extra fields are in the "User Fields" category it will actually be open and moved to the top.

   Changing the test to use an alter remove the extra fields from the list and then add them back. This will cause the "User fields" category to be closed and at the bottom and then open and at the top.

I started from #62 because of reason in #66 I didn't include @bnjmnm's patch in #65

Tests failed because I removed LayoutBuilderTestTrait but I forgot that AjaxBlockTest was also using this trait.

I removed this from trait because is less generic after removing the wait when you might want to check if a block link is visible and that would include a wait for the list to appear.

Looking at this again the method name or doc says nothing about waiting. I was just thinking because it had a wait it in before. With an assert on visibility no caller should assume it also has wait.

Pleased to see that #66 recommends scoping the css changes to layout builder - this provides much more flexibility and I definitely prefer this styling to my prior iteration.
Change is CSS + adding some classes to make it easier to target.
Before

After

Hmm, I guess I have one remark (actually it was from one of our clients). It's nice that groups don't need an additional request to the server, but 'Create custom block' does and has a different workflow. Just saying, don't want to block this at all :)

@swentel thanks for taking a look.

That is true but not related to this issue

That happened already in #2968500: Change inline blocks workflow in Layout Builder to match mocks

We could create a separate issue though to optimize that. We could already return the list of "Create [block type]"(if more than 1) and just not display it until the user clicked "Create custom block". So to the user the workflow would be the same just not another trip to the server.

But again that should be a different issue.

Rerolled patch #70.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
@@ -92,4 +96,136 @@ protected function assertModifiedLayout($path) {
+    $assert_session->addressEquals(static::FIELD_UI_PREFIX . '/display-layout/default');
...
+    $this->drupalGet(static::FIELD_UI_PREFIX . '/display-layout/default');

These are now '/display/default/layout'

1. Updated the admin paths per #75
2. Added openAllBlockCategories() to LayoutBuilderTestTrait, which provides tests an easy way to un-hide the block list.
3. A few assertWaitOnAjaxRequest() calls were failing. Replaced those with waitForElementVisible, to look for something that would be returned on the Ajax request, and the tests passed.

How does this work when the primary way of authoring content for a layout is via inline blocks? In my case, content "Content fields" are actually just metadata which is almost always irrelevant to the content that ends up in the actual layout.

The issues addressed in #76.3 were specific to my local environment, not the reroll.
Adding a new patch and diffs for #73 and #76

#78 @Sam152 "Create custom block"(which is what in code is called "inline-block" is still the top link. If you have 1 Custom Block type this is will take you directly add form if you have more than 1 it will take you to a list of block types.

As I mentioned in #72 we could open a different to optimize how many server calls this takes.

But this issue doesn't change that "inline blocks" are still promoted as way to make layouts.

Ah indeed. We have a large library of reusable custom blocks, so I believe those would also show up in a large list. I am currently experimenting with this sandbox for truncating large list of block plugins: https://www.drupal.org/sandbox/sam/3033243

This is looking good! Just some nits.

1. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -137,16 +161,114 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +   *   The delta of the section to splice.
   

   Idk that we're splicing any more.

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -137,16 +161,114 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +      $is_primary_block = function ($block_id) {
   ...
   +      $primary_block_ids = array_filter($block_ids, $is_primary_block);
   

   Might be personal preference, but it feels weird to have the array_filter callback split out to a local variable instead of inline. Usually to me that implies it is reused, but it is not here.

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -137,16 +161,114 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +        $block_id_parts = explode(':', $block_id);
   
   +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -36,6 +36,17 @@ function layout_builder_test_plugin_filter_block__layout_builder_alter(array &$d
   +      list($block_type) = explode(':', $plugin_id);
   

   Use \Drupal\Component\Plugin\PluginBase::DERIVATIVE_SEPARATOR here instead of ':'.

   I know LB does this wrong in other places, but let's stop propagating it further

4. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -137,16 +161,114 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +      $block_ids = array_keys($blocks);
   ...
   +      $secondary_block_ids = array_diff($block_ids, $primary_block_ids);
   +      $primary_block_links = $this->getBlockLinks($section_storage, $delta, $region, array_intersect_key($blocks, array_flip($primary_block_ids)));
   +      $secondary_block_links = $this->getBlockLinks($section_storage, $delta, $region, array_intersect_key($blocks, array_flip($secondary_block_ids)));
   

   You can use the ARRAY_FILTER_USE_KEY flag for array_filter and not have to do the array_keys or array_flip-ing, and that array_diff will be an array_diff_key

Fixed #83.

I thought I was going to be smarter than Tim and use array_diff_assoc instead of array_diff_key as suggested. It turns out array_diff can't deal with multidimensional arrays.

Aha, but if you want we have a DiffArray utility class that can handle that!

#83.4 ARRAY_FILTER_USE_KEY was only added in PHP 5.6, so I'm reverting that part.

Ahhhh you're absolutely right. Thanks!

Just because I've run into this a few times recently: The two test errors look like they're happening because the blocks are not immediately available to click due to being collapsed inside a category. The easiest way to address is will probably be using openAllBlockCategories(); from LayoutBuilderTestTrait to open all categories immediately after opening the "Add block" dialog.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestTrait.php
   @@ -0,0 +1,48 @@
   +  protected function openAllBlockCategories() {
   +    $open_all = 'document.querySelectorAll("#drupal-off-canvas details").forEach((element) => {element.setAttribute("open", true);});';
   +    $this->getSession()->executeScript($open_all);
   +  }
   

   I think we should avoid executeScript() if we can when writting JS tests. Since we are doing emulating what the user could do.

   I am changing this is actually click all closed categories to open them.

2. Fixed the test failures

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestTrait.php
@@ -21,8 +21,14 @@ protected function clickBlockCategory($category) {
+    foreach ($page->findAll('css', '#drupal-off-canvas .layout-builder-block-categories summary') as $closed_category_summary) {
+      if ($closed_category_summary->getAttribute('aria-expanded') === 'false') {
+        $closed_category_summary->click();

FYI I tried here to add [aria-expanded="false"] to the CSS selector in the foreach but this does not work because after the first click it changes the xpath that NodeElement uses and then the click() won't work for other elements because they no longer exist at the exact xpath that Mink uses to keep track of the elements.

I've done too much on this issue to RTBC it, but want to mention that the changes @tedbow made to the tests I wrote are definitely improvements. The end result is essentially the same, but it now accomplishes it by reproducing what a user would do so -- that == a better test.

Confirmed its OK for me to RTBC since the none of the changes since the last one came from me.

Does #3034347: Update the Layout Builder UI classes to implement BEM naming conventions affect the CSS here at all (i.e. are we still using the correct class names, and do any added class names also follow the standard)?

Reroll

Reroll passed - back to RTBC

Re: #97, no selectors in this ticket are impacted by the changes in #3034347: Update the Layout Builder UI classes to implement BEM naming conventions . This reroll just accounts for new line positions.

But the new selectors also don't appear to be following BEM.

The CSS included in this patch is definitely not BEM, I agree with @xjm.

Setting to NW to review CSS

Refactored CSS for BEM and added a few classes to ChooseBlockController to facilitate that.

The CSS looks good to me, and if this passes tests, I'd feel fine re-RTBCing this once Lauri signs off.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -114,3 +114,40 @@
   +  background-color: #444;
   +  color: #fff;
   ...
   +  color: #ddd;
   

   The hover styles are very subtle. Has this been run past the accessibility team?

2. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -137,23 +161,126 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +              'class' => ['layout-builder-block-categories__secondary-blocks__summary'],
   

   Nesting elements like this isn't supported by BEM (see official documentation). I suggest that we add new block element class called layout-builder-secondary-blocks and this will then become layout-builder-secondary-blocks__summary.

#106.1 -- Hover styling has been changed to use the default approach for details elements in off-canvas, where the background color changes instead of the text color. There are still some visual differences to distinguish primary from secondary, so tagging this with Needs Accessibility Review to get confirmation the approach is OK particularly since the default state has a bg color that matches the container.

#106.2 -- Changed the selectors so there isn't nesting

The IS mentions removing blocks like "Help" and "Main page content".
That was the initial scope of this issue.

This issue has a very separate and complex solution now, but one that does not address the problem as originally defined.

Update issue summary

Updated screenshots

MIslabeled screenshots

The "before" screenshot is stale (missing the filter textfield and the "create custom block" part)

Retitling as we're no longer removing anything.

Adding a new "before" screenshot to the IS.

By default close all categories except for the entity type categories which contain the field blocks.

Be aware that details/summary in the off-canvas dialog is currently broken. If you're going to have details closed by default, then #3037121: Disclosure triangle is missing in details/summary groups in off-canvas dialog in Firefox is a must-fix.

"More" summary needs to be disambiguated.

Recommend calling these "More user fields", etc

Re. #107

There are still some visual differences to distinguish primary from secondary, so tagging this with Needs Accessibility Review to get confirmation the approach is OK

I don't understand what this means. What's the purpose of primary/secondary here? (i.e. the semantic difference between them)

If the difference is important, it must not be conveyed by colour alone, per WCAG "Use of Color" (level A),

. I'm not sure what is causing this, but for some reason, this patch removes most of the block groups from tab order on Chrome:

Re. #120 - I replicated the problem. Firefox is affected too.

The image in 120 doesn't really help. You used the Focus Order Favlet, which doesn't pick up on all tabbable elements. Specifically it doesn't pick up the summary element.

The patch here isn't the problem, I can replicate it without using the changes here. What I found was that details/summary groups which are initially closed are kept out of the tabbing order in the off-canvas dialog.

The tiny patch attached here demonstrates this. The only change is it makes all details elements closed by default in ChooseBlockController. Try to replicate these steps:

1. Click add block, to enter the Choose a block dialog.
2. None of the details groups are tabbable. Tabbing cyles around the block filter. create custom block, and the dialog close button.
3. Weird thing 1: click on a closed details group (e.g. Help) using a mouse, and it will open. Now the keyboard tabbing order includes all of the closed details groups before Help, but NOT the groups after Help. Try tabbing forwards or shift-tabbing backwards.
4. Weird thing 2: now tab to the Help group, and press space to close it. No details groups are open. Now you can tab forward through the rest of the closed groups, which weren't tabbable before!
5. Weird thing 3: Keep tabbing forward, and you will leave the dialog and tab through the rest of the page. The dialog tabbing constraint has broken.
6. Keep tabbing forward through the entire page. Lots of tabs, I know. Eventually you will enter the off canvas dialog again.
7. Weird thing 4: Now none of the closed details groups are tabbable, and tabbing is constrained to the block filter, create custom block, and the dialog close button.

I replicated the same behaviour with Chrome72 and Firefox65 (on Kubuntu Linux 18.10 fwiw).

Note: Views UI has some details groups which are initially closed, in the floating dialog type. These don't seem to be affected. So maybe something is different about the way off-canvas dialog constrains tabbing?

That's all I got so far. It looks like this patch has uncovered a pre-existing bug.

Adding accessibility blockers to the issue summary:

• #3037121: Disclosure triangle is missing in details/summary groups in off-canvas dialog in Firefox
• The bug in #120-121. This sounds like it could affect details elements in any off-canvas dialog, so it should be spun out as a separate issue.

Changes made:

1. From #117: Included fix for missing disclosure triangle in Firefox with @todoreferencing #3037121: Disclosure triangle is missing in details/summary groups in off-canvas dialog in Firefox.
2. From #119: Added afont-weight: normal; rule to the secondary category summary so there is more-than-color visual distinction between it and the primary category details element is is nested in
3. Added several CSS rules that addressed IE display problems as a result of the collapse.js details polyfill. Accompanied by a @todo referencing #1839158: Replace collapse.js with a proper polyfill for <details>. A png was also added that overrides the default (poor contrast) image used by the polyfill. Before and after screenshots are attached
4. Changed the classname js-layout-builder-categories so it matched the naming of the non js- prefixed class it is accompanied by.
5. From #120 The tab navigation issue is addressed, accompanied by a @todo to an issue I created #3038336: When jQuery UI constrains tabbing it does not consider summary elements
6. From #118: Disambiguated the secondary category links so they are now labeled "More (category name)" instead of just "More". Updated screenshots in IS with these changes.
7. Removed accessibility blockers from IS as this patch includes issue-specific fixes with @todos to remove those fixes when the broader issue is addressed.

Restoring the a11y blockers to the summary - please leave them in place, even if you have attempted to fix them. There's an important difference between "accessibility issues have been identified, then fixed" and "accessibility issues have been identified, then lost".

#123.2 - I still don't know what primary and secondary refer to here. Can someone answer my question from #119?

Primary and secondary categories are referring to the <details> elements in the block listing. Secondary categories are nested inside primary categories.

#127 - thanks, but how does that relate to #107? What was the part you wanted accessibility review for? Something to do with background colours?

#123 - parts 1 and 3: I don't think it's a good idea to fix things here, just for the sake of this feature request.

#123.1:

Included fix for missing disclosure triangle in Firefox with @todo referencing #3037121: Disclosure triangle is missing in details/summary groups in off-canvas dialog in Firefox

I can't find this @todo - the string "3037121" doesn't appear in patch #123. What is the fix that you used? Does it addrees the bug for all details groups inside an off-canvas dialog?

#123.3:

+/**
+ * The following three rules are for the IE <details> polyfill provided by
+ * collapse.js (note the collapse-processed class).
+ *
+ * @todo Remove or re-work in https://www.drupal.org/node/1839158
+ *
+ * @see /core/misc/collapse.es6.js
+ */
+#drupal-off-canvas .collapse-processed .layout-builder-block-categories__summary a {
+  font-weight: bold;
+}
+
+#drupal-off-canvas .collapse-processed .layout-builder-block-categories__summary a,
+#drupal-off-canvas .collapse-processed .layout-builder-secondary-blocks__summary a {
+  color: #eee;
+  background: transparent;
+}
+
+#drupal-off-canvas .collapse-processed > .layout-builder-block-categories__summary:before,
+#drupal-off-canvas .collapse-processed > .layout-builder-secondary-blocks__summary:before {
+  background-image: url('../images/layout-builder-disclosure-triangle.png');
+}

The collapse.js polyfill is a red herring here; it doesn't attempt to provide a disclosure triangle icon. The icon comes from the Classy theme.

Is there any reason why this can't use #drupal-off-canvas .collapse-processed > summary:before {}, and live inside core/misc/dialog/off-canvas.details.css? The selector it is overriding is .collapse-processed > summary:before {}, which isn't layout-builder specific. As it stands, this code only attempts to fix the appearance issue for a specific form controller. That's a great disservice to developers of contrib and custom modules, who have a reasonable expectation of being able to use the same elements as layout builder does. It's at odds with our principle of making software that anyone can use.

I still think #3037121: Disclosure triangle is missing in details/summary groups in off-canvas dialog in Firefox should be a blocker for this issue.

#123.5:

         'class' => ['layout-builder-block-categories__summary'],
+        // @todo setting tabindex should not be necessary after
+        //   https://www.drupal.org/node/3038336
+        'tabindex' => 0,
       ],

Did this get cross-browser manual testing? In IE and Edge, the summary element isn't the operable control. We must not add tabindex="0" to an element that isn't operable. I'm concerned that this will double the number of tab-stops, but only half of them will actually do anything.

Update: I tested patch #123 in IE11 + Edge 42, on Windows 10. The double tab-stop problem does indeed happen. For sighted keyboard users, it's effectively a control which only works half the time. The workaround (carefully count the tab stops) would be hard to discover IMO. We can't use this approach.

@andrewmacpherson it is common to provide temporary solutions that workaround known core bugs which would be out-of-scope to fix in this issue.
The above @todo is sufficient for this issue.

Per #130, tabindex="0" is not a viable fix. It makes things worse in 2 browsers that aren't affected in the first place.

#126

#123.2 - I still don't know what primary and secondary refer to here. Can someone answer my question from #119?

Thanks Andrew! In this case, if you have the chance, could you just test the patch as-is and see if there are any specific problems that stand out in general? That's probably what will help the most at this point.

#129

• Disclosure triangle issue fix is in core now #3037121: Disclosure triangle is missing in details/summary groups in off-canvas dialog in Firefox

• Is there any reason why this can't use #drupal-off-canvas .collapse-processed > summary:before {}, and live inside core/misc/dialog/off-canvas.details.css? The selector it is overriding is .collapse-processed > summary:before {}, which isn't layout-builder specific.

  The scope of this issue is Layout Builder. An issue already exists (however neglected it might be) to replace collapse.js #1839158: Replace collapse.js with a proper polyfill for <details>. The CSS in classy that adds the triangle includes the class .collapse-processed , a class that is only present on elements modified by collapse.js.

• Did this get cross-browser manual testing? In IE and Edge, the summary element isn't the operable control. We must not add tabindex="0" to an element that isn't operable. I'm concerned that this will double the number of tab-stops, but only half of them will actually do anything.

  I'm very glad you caught that. This helped me narrow it down to a jQuery UI bug. There is a Drupal-wide fix in #3038336: When jQuery UI constrains tabbing it does not consider summary elements that is currently awaiting review. A Layout-Builder specific version with a @todo was also added - a precaution in case there are obstacles in getting 3038336 committed.

It's just a reroll.
needs review

Tried to reroll the latest working patch #133.
Can't recreate interdiff file.

Rerolled #133 and the interdiff is based on that. Some additional changes:

1. Removed the tab navigation fix with accompanying @todo. The issue that addresses this is ready for review and is preferable to how it is implemented in this issue #3038336: When jQuery UI constrains tabbing it does not consider summary elements
2. Updated several tests to reflect changes in this patch, particularly accounting for the need to open categories when adding certain blocks.
3. Added deprecation warnings to the two new constructor args added to ChooseBlockController
4. Updated CSS to meet lint standards and copied the changes to the layout builder css in the Stable theme.

I am cruising through this one (sorry) and I don't want to discount the work and decision making here but I do feel like adding the "more" subsection to every category is just really confusing and... sadly... oh so very Drupaly of us.

My opinion coming: A simple and elegant solution here is really to just dial it back.

1. Change 'TRUE' to 'FALSE' on line 162 of ChooseBlockController
2. Optionally: Add some JS that uses local storage to remember which categories were last open.
3. Optional, but important: Use layout_builder_restrictions and safe-list only the blocks useful to your clients. (see: #2998848)

I am adding a patch here that does the true/false swap and I'll go ahead and 'DO-NOT-TEST' the title to let y'all do what you want to do on this.

I know I am coming in with a bomb here and you have every right to ignore it.

For Acquia's Demo Framework, unless I see something better than what's currently proposed, I'm just going to do #1 and #3 mentioned above, because its simple and I don't really understand what you all are trying to do here with these psuedo-category "More" bump out subsections.

It's making me think and I don't believe that's what we want users to have to do here?

Just wanted to add the other issue that has this patch. Seems like a nice fix.
https://www.drupal.org/project/drupal/issues/3069446

Just confirming that this came up in a recent UX test that Acquia did of Layout Builder, reaffirming the "major" status here. The block listing was overwhelming to users and they were unable to use it to find what they were looking for.

> The block listing was overwhelming to users and they were unable to use it to find what they were looking for.

Have we determined then that the best solution is to hide some blocks while making others available via the extra "More" fieldset? You've just described a problem but the issue still is proposed in the form of a solution. Its right in the title, "by hiding uncommon block plugins"

I don't disagree that hiding some things is a possible solution, but its weird to me that this was worked on for so long without anyone really questioning the UX decision proposed initially.

No, that's totally fair. It might be worth splitting out a separate issue to talk about solving the "OMG SO MANY THINGS" problem separately, and see if something like this or #3069446: Layout builder's "Add Block" sidebar menu UX improvements or something else entirely is the best way to handle that.

Did that: #3073648: The list of available blocks in Layout Builder is overwhelming to users

Let's postpone this issue on a more holistic discussion of how to tackle this.

For the people looking for a solution: The Block list override module does a good job in cleaning up the layout builder list.