Ship layouts that make sense with Layout Builder's concept of sections

These sound reasonable to me.

How should we hide the old ones from LB?

Meanwhile, patch for the new ones.

I think the fourcol one is still broken but my laptop just went through an adventure that is scarily like a logic board and/or encrypted HD partition corruption, so trying to upload this before the work is lost..

In the process of copying the layouts I noticed there are some old @todo re: flex layouts not being able to be stabilized until IE9 and 10 support is dropped. But no explicit followup issue seems to have been listed, so I'm not sure they ever got copied over.

Sure, that makes sense. I'll move them (and remove the Stable addition).

Moving into layout builder and using hook_plugin_filter_TYPE__CONSUMER_alter to remove the unneeded layout_discovery layouts for the "choose layout section". Not sure if we should remove them from all consumers.

This might be outside the scope of this issue a little bit, but I wonder if instead of hardcoding the Layouts to be filtered out we should instead provide some kind of configuration for hiding Layouts?

For example perhaps a site has other modules which provide other layouts but are not needed and so are cluttering up the "Choose a layout" list.

That functionality is provided by http://www.drupal.org/project/layout_builder_restrictions

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -57,7 +57,8 @@ function layout_builder_entity_type_alter(array &$entity_types) {
   - * Implements hook_form_FORM_ID_alter() for \Drupal\field_ui\Form\EntityFormDisplayEditForm.
   + * Implements hook_form_FORM_ID_alter() for
   + * \Drupal\field_ui\Form\EntityFormDisplayEditForm.
   

   Unrelated change. Not sure it got in. Reversed.

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -202,3 +203,35 @@ function layout_builder_block_content_access(EntityInterface $entity, $operation
   +function layout_builder_plugin_filter_layout__layout_builder_alter(array &$definitions, array $extra) {
   +  // Remove layouts provide by layout discovery that are no longer needed
   +  // because of layouts provided by this module.
   +  $duplicate_layouts = [
   

   Should we actually remove these layouts for all consumers?

   You effectively these older layouts by combining the layouts provided by layout_builder. If you are using layout_builder along with other modules that use layout plugins it will be weird that you see different layouts in different places when the person using the layouts might not be the site builder so doesn't know anything about the different modules.

   On the other hand layout_discovery is stable so sites might have workflows around these layouts.

   I think there are couple options.

   1. Add a BC_ flag that will hide the old these layouts for all consumers for sites that turn on layout_builder after this issue is committed. Similar to what we have done for normalizers in the serialization module.

   2. Encourage contrib modules to also hide these layouts in the same way. There are not too many modules right now that use layout plugins I would think.

3. +++ b/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css
   @@ -0,0 +1,22 @@
   + */
   +.layout--fourcol-section {
   

   Need an empty line here.

I'm not sure that we want to commit to shipping all of these exact layouts, especially the different widths of the 3 column.
We talked a while ago about a 3 column layout with a configuration option to pick between different widths, and varying the CSS classes by that option. I think we should revisit that here, and make sure we really like the layout options we're providing.

Ok like the idea of #16. Then could switch width of a two column layout after it is placed.

forgot the patch

1. +++ b/core/modules/layout_builder/layout_builder.layouts.yml
   @@ -0,0 +1,52 @@
   +  class: '\Drupal\layout_builder\Plugin\Layout\TwoColumnLayout'
   

   Instead of using the annotation in the plugin class I used the 'class' property in the yml file.

   I think it is better because you are able to quickly all the layouts the module provides in 1 yml file.

2. +++ b/core/modules/layout_builder/src/Plugin/Layout/TwoColumnLayout.php
   @@ -0,0 +1,26 @@
   +      '50-50' => '50%/50%',
   +      '34-66' => '34%/66%',
   +      '66-34' => '66%/34%',
   +      '25-75' => '25%/75',
   +      '75-25' => '75%/25%',
   

   I added all this different options for the 2 column layout. Because since we already the ability to have as narrow columns of 25% in the for column you might as well have it here in the 2 column to line up.

3. I just noticed that we should probably change
   34%/66% to 33%/67% so that first column would line up if it was below a 3 column 33%/34%/33%.
   Same for 66%/33%.

fixed #19.3

Fix test failure because we now need save layout configuration form when adding a section.

1. +++ b/core/modules/layout_builder/layouts/threecol_section/layout--threecol-section.html.twig
   @@ -0,0 +1,43 @@
   +  set classes = [
   +    'layout',
   +    'layout--threecol-25-50-25-section',
   +  ]
   

   Don't need this anymore since they are added in the class.

2. +++ b/core/modules/layout_builder/src/Plugin/Layout/ThreeColumnLayout.php
   @@ -0,0 +1,22 @@
   +      '25-50-25' => '25%/50%/25%',
   +      '33-34-33' => '33%/34%/33%',
   

   Adding 25%/25%/50% and %50/25%/25%

   I think that covers all the 1/3, 1/4 splits across 2 and 3 columns

another self review. ⚡️Test passing!

1. +++ b/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css
   @@ -0,0 +1,23 @@
   +.layout--fourcol-section > .layout__region,
   +.layout--fourcol-section > .layout__region--second {
   

   We don't need the second selector line. Maybe copy/paste error?

2. +++ b/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css
   @@ -0,0 +1,23 @@
   +  .layout--fourcol-section > .layout__region--first,
   +  .layout--fourcol-section > .layout__region--second,
   +  .layout--fourcol-section > .layout__region--third,
   +  .layout--fourcol-section > .layout__region--fourth {
   

   All of these selectors can be replaced by
   .layout--fourcol-section > .layout__region

3. +++ b/core/modules/layout_builder/layouts/threecol_section/threecol_section.css
   @@ -0,0 +1,37 @@
   +  .layout--threecol-section > .layout__region,
   +.layout--threecol-section > .layout__region--second {
   

   Don't need second selector

4. +++ b/core/modules/layout_builder/src/Plugin/Layout/MultiWidthLayoutBase.php
   @@ -0,0 +1,78 @@
   + * Base class of layouts with configurable widths.
   

   Need to add @interal

   Will this always be internal or only until module stable. Seems like it would handy for other modules to extend.

5. +++ b/core/modules/layout_builder/src/Plugin/Layout/ThreeColumnLayout.php
   @@ -0,0 +1,24 @@
   +  }
   +}
   

   Need blank line

smaller images for summary

And fixed one of the labels for the width options missing a "%"

Added testing for correct layouts available.

Just adding my support here for #16, we can provide configuration for Layouts, so may as well make full use of them and provide as much customisation as possible, Bootstrap Layouts is a good example of making full use of configuration to style the Layout.

Signing off on this, but want @DyanneNova's signoff as well.

Reroll

This looks good to me!

1. I don't feel I can commit this since I authored the initial patch (even though it's evolved a lot), but +1 for the hook_plugin_filter_TYPE__CONSUMER_alter() approach. I think that's a good way to address this issue without a BC break, and glad to see it being included in the same patch.

2. +++ b/core/modules/layout_builder/layouts/fourcol_section/layout--fourcol-section.html.twig
   @@ -0,0 +1,49 @@
   + * This template provides a three column 25%-25%-25%-25% display layout.
   

   Actually, it's a four-column layout. This typo may or may not ahem, cough, ahem date back to my initial patch.

1. I think a small CR might be nice here for the additions.

2. +++ b/core/modules/layout_builder/src/Plugin/Layout/MultiWidthLayoutBase.php
   @@ -0,0 +1,84 @@
   +abstract class MultiWidthLayoutBase extends LayoutDefault implements PluginFormInterface {
   

   This is a sweet feature.

   I think it needs test coverage though; NW for that.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -675,4 +679,31 @@ public function testSimpleConfigBasedLayout() {
   +  protected function assertCorrectLayouts() {
   

   Glad to see this tested!
    

4. What happens when someone edits an existing Layout-Builder-built layout given this change? As far as I recall the layout of a section can't be changed other than deleting and re-adding the section, so I don't think we have any data integrity risk. And we also certainly can't try to provide an upgrade path screwing with people's section layouts because we have no way of knowing what they actually wanted or not. But it might be weird for a layout to have a mix of sections with layouts that aren't visible anymore and these. TLDR we're probably making the right choices, but I don't see this aspect discussed yet, so let's discuss/document that the UX etc. is OK when you have existing layouts using the old Layout Discovery section templates.

I can answer #34.4:
The nice thing about hook_plugin_filter_TYPE__CONSUMER_alter() is that it's runtime only, and also only for the places that call \Drupal\Core\Plugin\FilteredPluginManagerInterface::getFilteredDefinitions(). And only UIs should use that.
So yes, those old layouts won't be "visible" in the sidebar anymore when selecting new ones. But they will still function 100%.
Additionally, this hook only removes them when the provider is layout_discovery, so any custom code could permanently restore them by altering the plugin definitions to switch the provider to their own custom module.

Yep, so just manual testing of the UX with a layout that has the old section templates?

@xjm thanks for the review!

Re #34.2

Here is a test. Adds the 2 & 3 column sections and cycles through all the width options making sure that correct class is applied for each section.

Added change record: https://www.drupal.org/node/3020140

Thanks!

Is the title of the change record the most accurate description of the change? "Layout Builder now ships with single column sections instead of stacked sections", should that say "single row sections" instead of "single column sections"?

@AaronMcHale thanks good catch. Fixed it.

Great, thanks tedbow, looks good now :)

1. Manually tested and this works great.

2. +++ b/core/modules/layout_builder/layouts/fourcol_section/layout--fourcol-section.html.twig
   @@ -0,0 +1,49 @@
   + * This template provides a three column 25%-25%-25%-25% display layout.
   

   three = four :) ah... there is more see comment below.

3. diff --git a/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css b/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css
   index 6f35a7f33e..de1f3f87f3 100644
   --- a/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css
   +++ b/core/modules/layout_builder/layouts/fourcol_section/fourcol_section.css
   @@ -1,6 +1,6 @@
    /*
     * @file
   - * Provides the layout styles for layout_fourcol_section.
   + * Provides the layout styles for four-column layout section.
     */
    
    .layout--fourcol-section {
   diff --git a/core/modules/layout_builder/layouts/fourcol_section/layout--fourcol-section.html.twig b/core/modules/layout_builder/layouts/fourcol_section/layout--fourcol-section.html.twig
   index 6dd8914fc0..9a380f76e0 100644
   --- a/core/modules/layout_builder/layouts/fourcol_section/layout--fourcol-section.html.twig
   +++ b/core/modules/layout_builder/layouts/fourcol_section/layout--fourcol-section.html.twig
   @@ -1,9 +1,7 @@
    {#
    /**
     * @file
   - * Default theme implementation for a four-column layout.
   - *
   - * This template provides a three column 25%-25%-25%-25% display layout.
   + * Default theme implementation for a four-column 25%-25%-25%-25% layout.
     *
     * Available variables:
     * - content: The content for this layout.
   diff --git a/core/modules/layout_builder/layouts/threecol_section/layout--threecol-section.html.twig b/core/modules/layout_builder/layouts/threecol_section/layout--threecol-section.html.twig
   index 356222e327..1311f28ab2 100644
   --- a/core/modules/layout_builder/layouts/threecol_section/layout--threecol-section.html.twig
   +++ b/core/modules/layout_builder/layouts/threecol_section/layout--threecol-section.html.twig
   @@ -3,8 +3,6 @@
     * @file
     * Default theme implementation for a three-column layout.
     *
   - * This template provides a three column display layout.
   - *
     * Available variables:
     * - content: The content for this layout.
     * - attributes: HTML attributes for the layout <div>.
   diff --git a/core/modules/layout_builder/layouts/threecol_section/threecol_section.css b/core/modules/layout_builder/layouts/threecol_section/threecol_section.css
   index e92c0cc433..49b5578bfc 100644
   --- a/core/modules/layout_builder/layouts/threecol_section/threecol_section.css
   +++ b/core/modules/layout_builder/layouts/threecol_section/threecol_section.css
   @@ -1,6 +1,6 @@
    /*
     * @file
   - * Provides the layout styles for layout_threecol_section.
   + * Provides the layout styles for three-column layout section.
     */
    
    .layout--threecol-section {
   diff --git a/core/modules/layout_builder/layouts/twocol_section/twocol_section.css b/core/modules/layout_builder/layouts/twocol_section/twocol_section.css
   index 3116e5f47f..0d21223350 100644
   --- a/core/modules/layout_builder/layouts/twocol_section/twocol_section.css
   +++ b/core/modules/layout_builder/layouts/twocol_section/twocol_section.css
   @@ -1,9 +1,6 @@
    /*
     * @file
   - * Provides the layout styles for layout_twocol_section.
   - *
   - * @todo Using display: flex requires https://www.drupal.org/node/2842298 to be
   - * in before this can be marked as stable.
   + * Provides the layout styles for two-column layout section.
     */
    
    .layout--twocol-section {
   

   I think we need to apply the changes above to make the patch comments in css and templates consistent. Plus the @todo is not the necessary as that patch has landed - #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/TestMultiWidthLayoutsTest.php
   @@ -0,0 +1,137 @@
   +   * @todo Remove in https://www.drupal.org/node/2892440.
   

   Nice that this is here - shame we've not landed that yet.

Agreed with the diff in #44.3, applied that verbatim and then removed all of the links pointing to #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x
Leaving RTBC.

(interdiff.txt is my changes, interdiff-full.txt includes #44.3)

Crediting @EclipseGc for creating the issue and offline discussions.
Crediting @AaronMcHale, and myself for issue review and comments.
Crediting @DyanneNova for issue sign off as layout initiative coordinator.

Committed 6cb2397 and pushed to 8.7.x. Thanks!

This introduced test failures on PHP5:
https://www.drupal.org/pift-ci-job/1166382
https://www.drupal.org/pift-ci-job/1165837

Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testLayoutBuilderUi
Exception: Strict warning: Drupal\Core\Plugin\PluginBase and Drupal\Core\StringTranslation\StringTranslationTrait define the same property ($stringTranslation) in the composition of Drupal\layout_builder\Plugin\Layout\MultiWidthLayoutBase. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. Class was composed
require()() (Line: 84)

Good catch, this one is thankfully a REALLY easy fix!

Crediting @tacituseu for discovering and reporting #48.

Pushed to 8.7.x.