Add a container option

Great patch thank you.

Works as designed. +1 RTBC

+++ b/src/Plugin/Layout/BootstrapLayoutsBase.php
@@ -44,6 +44,8 @@ class BootstrapLayoutsBase extends LayoutDefault implements PluginFormInterface
+        'section_attributes' => '',

+++ b/templates/3.0.0/bs-1col-stacked.html.twig
@@ -18,6 +20,10 @@
+  <div{{ section_attributes }}>
+    <div class="container">

This isn't just adding a "container", but also a "section".

I think this is a bit overkill. Or perhaps it's backward.

The layout/wrapper itself should be able to be the "section". Then the container should be just inside the layout/wrapper surrounding the regions.

Anything more seems like a site-specific requirement.

This should become container_attributes and default to having a container class inside it.

See #6.

+++ b/templates/3.0.0/bs-1col-stacked.html.twig
@@ -18,6 +20,10 @@
+{% if container %}
+  <div{{ section_attributes }}>
+    <div class="container">
+{% endif %}

The need for a new "section" element/wrapper is unnecessary.

Just add <div class="container"> inside the layout (which still needs its own attributes variable) that wraps the layout's regions.

Then you can use the existing layout element and its attributes as what you're currently calling the "section".

Like I said in #6, you're doing this backwards.

I see.

Regardless, anything we do here will need to be added inside the existing layout wrapper element.

For a BC approach, we should probably just introduce two new settings/UI toggles (container and row), each named something like "Add separate container element" and "Add separate row element".

Then create new attributes (container_attributes and row_attributes) and accompanying UI customization for said elements and attributes as if they were their own regions.

Both of these customizations areas (likely additional vertical tabs) should only be shown if the relevant UI toggle is checked, thus this will need some #states code.

I think that will give anyone the flexibility to do whatever they desire.

Pardon me if I'm wrong but there is a difference between having a 'no container' wrapper or a 'container-fluid' wrapper: the latter has left and right paddings of 15px so the content is slightly away for the browser sides while the 'background' is still touching them.

So, shouldn't we have 3 options to choose from for the section class: nothing, 'container' or 'container-fluid'?

Hi Mark, the attached patch provides configuration for enabling/customizing the following wrappers:

• Row
• Container
• Wrapper (those cases that you need to use flexbox)

I added a checkbox "Add .... element". This way, you can configure and only enable to your liking. By default, it matches the current behavior of only adding the row element.

I know you mentioned earlier that we should reuse the existing layout element, but I wasn't able to figure out a way of adding nested elements into that one element so I created 3 separate elements in the template.

Let me know if you were thinking of going in a different direction?

Fixed issue with container items not displaying after save.

Found missing ending tags in the 1 column layout.

Thanks for the patches @albertski! Awesome work.

A few questions and concerns:

1. +++ b/bootstrap_layouts.module
   @@ -101,8 +101,24 @@ function _bootstrap_layouts_preprocess_layout(array &$variables) {
   +  $variables['wrapper'] = $layout->getSetting('layout.wrapper.wrapper_enabled', FALSE);
   +  $variables['wrapper_element'] = $layout->getSetting('layout.wrapper.wrapper_element', 'div');
   +  $wrapper_attributes = NestedArray::mergeDeep(_bootstrap_layouts_parse_attributes($layout->getSetting('layout.wrapper.wrapper_attributes', ''), $tokens), $layout->getSetting('layout.wrapper.wrapper_attributes', ''));
   +  $variables['wrapper_attributes'] = new Attribute($wrapper_attributes);
   +  $variables['wrapper_attributes']->addClass($layout->getSetting('layout.wrapper.wrapper_class', []));
   
   +++ b/templates/3.0.0/bs-1col-stacked.html.twig
   @@ -18,25 +25,46 @@
   +{% if wrapper %}
   +  <{{ wrapper_element }}{{ wrapper_attributes }}>
   +{% endif %}
   

   This seems like a ton of more work than is necessary. Can we just return the entire array of the wrapper settings? Then, in the template, you can just use dot notation (e.g. wrapper.enabled, wrapper.element, wrapper.attributes, etc.).

2. +++ b/bootstrap_layouts.module
   @@ -101,8 +101,24 @@ function _bootstrap_layouts_preprocess_layout(array &$variables) {
   +  $wrapper_attributes = NestedArray::mergeDeep(_bootstrap_layouts_parse_attributes($layout->getSetting('layout.wrapper.wrapper_attributes', ''), $tokens), $layout->getSetting('layout.wrapper.wrapper_attributes', ''));
   

   I will say this again:

   1) NestedArray cannot be used on attributes; classes have to be handled differently.

   2) We need a helper method that extracts the layout settings and when it encounters a property named "attributes" or ends with "attributes", that it automatically converts it into an attributes array.

   It could reduce duplication of this entire block of code down to just three lines; one for each: wrapper, container, row.

3. +++ b/src/Plugin/Layout/BootstrapLayoutsBase.php
   @@ -44,10 +44,25 @@ class BootstrapLayoutsBase extends LayoutDefault implements PluginFormInterface
   +          'wrapper_element' => 'section',
   

   Why is this being changed from div to section?

4. +++ b/src/Plugin/Layout/BootstrapLayoutsBase.php
   @@ -44,10 +44,25 @@ class BootstrapLayoutsBase extends LayoutDefault implements PluginFormInterface
   +        'row' => [
   +          'row_enabled' => TRUE,
   

   Why are these properties prefixed with the name of their parent? Isn't that the point of being able to nest arrays? I think there may be some confusion from when I originally suggested adding some additional properties, but they were just one or two at the root level. If we're going to nest them, there shouldn't be any need to prefix them too.

Thank you for your feedback @markcarver. I have updated the patch to fix the items you pointed out:

1 - I went ahead and went that route of returning the entire wrapper settings like you suggested. I agree. It makes better sense.

2 - I removed the NestedArray call and verified classes/ids are getting added. Also, I created the helper method `_bootstrap_layouts_process_attributes()`.

3 - I went back to `div`. Not sure why I decided to go to `section` earlier.

4 - Good idea on removing the prefix. There was no need for prefixes, and in fact, I was able to reduce duplication in the form by creating the form elements in an array.

I started to make a lot of review comments but decided it may just be easier for me to do a little bit of the work to show what I meant. So here's a patch that helps clean things up a bit more.

Mind you, I didn't actually test this on a running instance, just relied on PhpStorm's inspections to catch any errors. Needs to be reviewed and tested on a real instance.

1. +++ b/templates/3.0.0/bs-1col-stacked.html.twig
   @@ -18,25 +24,48 @@
   -  {% if top.content %}
   -  <{{ top.wrapper }}{{ top.attributes }}>
   

   While we're here, we may as well rename the "wrapper" region variable to "element" as well (if only to not confuse things more).

   https://git.drupalcode.org/project/bootstrap_layouts/-/blob/8.x-5.x/src/...

+++ b/bootstrap_layouts.module
@@ -101,16 +184,14 @@ function _bootstrap_layouts_preprocess_layout(array &$variables) {
+  $attributes = _bootstrap_layouts_merge_attributes($variables['attributes'], _bootstrap_layouts_create_wrapper_variables($layout)['attributes']);
+  $variables['wrapper'] = _bootstrap_layouts_create_wrapper_variables($layout, 'wrapper');

+++ b/templates/3.0.0/bs-1col-stacked.html.twig
@@ -18,25 +24,48 @@
-<{{ wrapper }}{{ attributes }}>
-  {{ title_suffix.contextual_links }}
+{% if wrapper.enabled %}
+  <{{ wrapper.element }}{{ wrapper.attributes }}>
+{% endif %}

These "attributes" are now actually being ignored since it's using the "wrapper.attributes" in templates.

So... this is technically a breaking change. Which means I'll have to bump the major version of the project. I'm ok with for the most part, but I'm still a little confused as to why this is necessary.

This is what I meant above by the fact that it appears that the "wrapper" wrapper being added here is redundant, the layout settings themselves are there for the "wrapper" around the regions.

Thanks, @markcarver. I took a look and tested and everything looks good. I had to make one minor fix: $value->value() instead of $value->getValue(). Also, on form submission I had some prefixes left over that I cleaned up.

I'm ok with for the most part, but I'm still a little confused as to why this is necessary.

I'm currently using this in Layout Builder, and I always run into cases where one section needs to be edge to edge and one section that needs to be in a container. With the current implementation (prior to the patch), you don't have that ability and only are stuck with whatever your entire page layout is set to.

If you are okay with going forward, I think we can proceed with renaming the wrapper to element for regions.

I had to make one minor fix: $value->value() instead of $value->getValue(). Also, on form submission I had some prefixes left over that I cleaned up.

Ah, great catch!

I'm currently using this in Layout Builder, and I always run into cases where one section needs to be edge to edge and one section that needs to be in a container.

That's what this extra container (and row) wrapper is supposed to be for. You can put the container class on one and not the other. If you need to style the element around that, you should target whatever the layout is actually in.

Layouts (in this sense) aren't typically supposed to be root-level components; there's always some sort of low-level element that encompasses them (entity, view, paragraph, field, block, theme region, whatever...).

I'm still not convinced having, potentially, 3 wrappers for every single layout is an absolute necessity. This is already bad enough with 2.

The only real reason I'm even considering the two (container and row) is mostly due to BS3 restrictions in its overuse of child and general/adjacent sibling combinators. Even BS4 (and soon to be BS5) have reduced the number of elements necessary with the advents of CSS (flex/grid).

If anything, I would argue that the majority who think of this as "needed" could just as easily redesign or improve it through discovering more modern CSS functionality.

I'm sorry, but given that cases of divitis are seemingly a neverending battle, I really don't want to help it come back if I can actively prevent it.

The goal of any project on d.o should be to provide quality code and the ability to extend it.

I'm looking at this with targeting the 99.999% of use cases.

On the off chance, if someone really believes (or even proves) that they need a 3rd or 4th wrapper they can always create one in a module or theme. It doesn't mean we have to give people an easier way to shoot themselves in the foot out-of-the-box; those unique situations should be handled on a case-by-case basis.

Is there a way that we can have a container wrapper around the row div with the current implementation? I didn't think there was a way to do that.

Let me see if I can convince you why we need the three wrapper options.

Row Only Option

<div class="row">
  <div class="col-sm-6">
    One of two columns
  </div>
  <div class="col-sm-6">
    One of two columns
  </div>
</div>

With the row only option (current implementation) we have to rely on the main page to add a container and then we would have to have every section be the same width.

Row and Container Option

<div class="container">
  <div class="bs-2col--row row">
    <div class="col-sm-6">
      One of two columns
    </div>
    <div class="col-sm-6">
      One of two columns
    </div>
  </div>
</div>

Now we have more control and we can have some sections wrapped in a container and some not.

Wrapper, Container and Row Option

<div class="wrapper">
  <div class="container">
    <div class="row">
      <div class="col-sm-6">
        One of two columns
      </div>
      <div class="col-sm-6">
        One of two columns
      </div>
    </div>
  </div>
</div>

Now we have the flexibility to add styling outside the container like add a background color.

Let me know what you think?

As I stated above, I understand the use cases for the first two; unfortunately unavoidable due to how Bootstrap works.

like add a background color

It's this third case (wrapper) that is redundant. Even with this use-case stated here, you used the word "like", implying this is still just a hypothetical "gain". The first two use-cases are based on actual requirements/restrictions of the Bootstrap framework and how it technically functions.

This hypothetical background can still be easily styled on the container it's in, whether it be a paragraph/field, block, theme region or whatever.

I'm also approaching this from years of experience where designers typically follow some sort of pattern. This usually means that if there is some variation, in say a background around an element, it's typically done so using numerated logic (i.e. first, last, odd/even, etc.). These can easily be targeted and coupled with modern CSS, allow an element to be styled as desired:

@mixin full-width-bg($background: #777) {
  position: relative;
  &, &::before, &::after {
    background: $background;
  }
  &::before {
    display: block;
    position: absolute;
    top: 0;
    right: 100%;
    width: 100vw;
  }
  &::after {
    display: block;
    position: absolute;
    top: 0;
    left: 100%;
    width: 100vw;
  }
}
// Style only first layout of a region.
.region .layout:first-of-type {
  @include full-width-bg();
}

// Style only last layout of a region.
.region .layout:last-of-type {
  @include full-width-bg();
}

// Style only even numbered layouts of a region.
.region .layout:nth-child(even) {
  @include full-width-bg();
}

// Style only odd numbered layouts of a region.
.region .layout:nth-child(odd) {
  @include full-width-bg();
}

Note: the above is likely to have some flaws, I simply wrote it here off the top of my head from work I've done in the past. This concept, while still a rather extremely rare use-case, does work though.

If a more complex design truly requires the use of a 3rd (or more) wrapper around a single layout, then that is the time to create a custom layout to be used on a site. This also has the added benefit of reducing complications during upgrades as the layout was designed with a specific use case in mind and is siloed outside a generic module.

If we keep adding wrappers just for the sake of adding wrappers, this module runs the risk of being 1) too customizable and ultimately confusing and 2) Non-Bootstrap specific, which one could already argue could already be the case; especially with this issue adding even more variety.

Sorry, I'm still not convinced that a "wrapper" is necessary now that there is a container; it's redundant.

+++ b/bootstrap_layouts.module
@@ -101,16 +184,14 @@ function _bootstrap_layouts_preprocess_layout(array &$variables) {
+  $attributes = _bootstrap_layouts_merge_attributes($variables['attributes'], _bootstrap_layouts_create_wrapper_variables($layout)['attributes']);

+++ b/templates/3.0.0/bs-1col-stacked.html.twig
@@ -18,25 +24,48 @@
-<{{ wrapper }}{{ attributes }}>

Also, these attributes are still being lost. They will need to be merged into the container now instead. We'll also need to add a caveat description to the form that indicates that disabling the container will prevent any attributes added to the layout (externally via a preprocess) from appearing.