Review layout CSS and markup

I think that this kind of feature inside the core should be added with at least some kind of responsive functionality. Currently the 25-50-25 (as example) layout is 25-50-25 layout also on 300px mobile. So, from my point of view as minimum the following tasks should be done:
1. Review that the CSS moved from Panels to Core is valid.
2. Add breakpoints for mobile and tablet and make the layouts for responsively.

The CSS should be fluid and adapt to whichever container it is on, not make assumptions about breakpoints. For that I wonder whether we could enable choosing the layout per breakpoint when breakpoint module is enabled.

@tim.plunkett could contrib provide such integration? (honest question, just wondering).

Fixed indentation on all 5 templates to reflect the standards used throughout core.

Added if statements to check for content.top and content.bottom on

• layout--twocol.html.twig
• layout--twocol-bricks.html.twig

I've also fixed the indentation on templates/layout.html.twig, hopefully that's in scope as well.

Here is the patch without the changes to templates/layout.html.twig, im thinking it is probably out of scope for this.

Also fixing the failing test.

// Templates

• I would suggest to have the layout class to be present, followed by the right class with modifier.

layout.html.twig

• The indenting is incorrect

layout--twocol-bricks.html.twig

• For several regions you use modifiers like --left_above, these should be --left-above
• For these regions i would suggest class naming as left-top, right-top, left-bottom, right-bottom. As the positioning is in the same way.

// Styling

• I am missing the file comments in the css files

Adding back the fixes to layout.html.twig.

There should be a failing test on Drupal\Tests\layout_discovery\Kernel\LayoutTest, which I can't figure out the solution to. If I add the same change made to layout.html.twig to the expected value on layout_test_1col_no_template, it should pass as far as I understand it, but it doesn't.

Wow thanks for that @tim.plunkett, that is weird indeed 0_o

Patch looks fine and fixes the points around the templates:

• Ensure templates have similar indentation
• Check that layouts with top and bottom regions have logic to hide them if there is no content

Setting to needs work so that someone looks at the CSS (looks fine to me, but surely there are others more into CSS than me nowadays)

I've cleaned up the classes and markup to be more consistent. I've also added a drop to single column below 40em. That should cover the most common use cases.

@DyanneNova thanks for working on this!

I'd like to make a suggestion, what do you think of making fullwidth the default for all regions, so that:

1. It is more in line with browser's default behaviour.
2. We can we get rid of the layout-region--fullwidth class.

In my opinion it would make extending the CSS of the layouts more intuitive.

Attached what I mean, just to the twocol layout as to explain a bit what I mean, what do you think?
So with min-width the default would be one col, and only when display is 40em or wider it'd set the two columns.

Cool but there's a problem when the column has no contents but doesn't collapse.

If your basis value is a percentage then the column will never shrink to anything less than that percentage, even if it's empty.

But if you set your basis to 0 on all they will collapse (see code below). Then if you change the grow values to 1 for the general region class, and 2 for the --middle class you achieve the same thing:

.layout-region 1/4 (grow 1) | .layout-region--middle 1/2 (grow 2) | .layout-region 1/4 (grow 1)

1/4, 1/2, 1/4 == 25%, 50%, 25%

But you also get:

.layout-region empty, 0 (because the basis is 0) | .layout-region--middle 2/3 (grow 2) | .layout-region 1/3 (grow 1)

Because the remaining number of grow values is 3 you get . 0, 2/3, 1/3 instead of an empty 25% column on the left, 50% in the middle and 25% on the right.

.layout--threecol > .layout-region {
    flex: 1 1 0;
}
.layout--threecol > .layout-region--middle {
    flex: 2 1 0;
}

Thanks @tkoleary that makes a lot of sense to me, and would make for more robust, versatile and therefor useful layouts.

My only concern would be about how this would impact the layout configuration UIs in Panels / DS. There could be disparity between what the user sees in the UI for inserting content into the layouts and what they see when they rendered, in the case when they only put one block one multiple column region for example. Not sure how these UIs are shaping up so far, but we should have them behave in the same manner. Perhaps it would not even be a problem already?

Attached a patch that:

1. Removes the rest of unnecessary fullwidth classes.
2. Implements the flex layout as per #21's suggestion, except for twocol-bricks layout.

For the twocol-bricks layout we cannot use the approach suggested in #21 with the current markup, so I am leaving it as is.

@Manuel Garcia

After some further experimentation I found that the solution I suggested in #21. Did not work exactly as I expected it to, especially the part about collapsing columns.

I think at this point perhaps @dyannenova should review. Her CSS skills are quite a bit better than mine.

Hi,

I'm adding a patch here that puts if statements around each region. I'm not sure that it makes sense to print empty regions (it's not what core does for page regions). It also means we can have CSS that will set regions to fill their available space if sibling regions are emtpy.

With that in mind ...I have also added some CSS to make regions in the 2 col layouts fill the width of the screen if a sibling region is empty. So, if 'left' has content but 'right' is empty, then 'left' will fill the available space.

I haven't cracked how we can make that work for the three-col layout just yet. I suspect we might need to add classes via if statements to achieve it.

Apologies, uploading a new patch - some of my development.services.yml customisations snuck into the last one.

Thanks @markconroy for working on this!
I like the idea in general, will do more testing etc when I manage some time for this =) Attaching the interdiff for now.

I think that using percentages is going to most closely represent what people expect from viewing the layout list. I tried to work out a list of rules to adjust the percentages based on which regions exist, but to do that we would need to make more assumptions that won’t be obvious to the user when selecting a layout, like changing an equal width three column layout to an equal width two column layout if one region isn’t used. I think we'll need to make more arbitrary decisions than we should to do that for every region combination. If we leave the percentages the same, themes can add their own layout specific code to adjust the widths as they see fit.

Here's a patch that collapses any region that isn't in use, but keeps the percentage widths of the other regions the same.

This should fix the patch fail.

Thanks you @DyanneNova I think the patch looks really good now. If anyone have any better ideas please chip in :)

If nobody comes up with something else, I think the next step would be to manually check each layout, taking screenshots.

• adding content to all
• adding content just some regions

So we can see if unused regions collapse and no other layout bugs appear.

Tested all layouts using Field Layout module, on a custom content type using a bunch of text fields:

• One-col
• Two-col
• Two-col-bricks
• Three column 25/50/25
• Three column 33/34/33

1. Columns stay the same if only one is filled in.
2. No markup is produced if a region is empty (yay)
3. Responsive behavior is mostly fine, and as a base i believe it would serve most cases.

I have only one thing I think everyone would benefit from having, gutters. Currently it looks like this:

Setting this to needs work because of this, other than that this is RTBC for me.

I think we should let themes handle gutters, then the gutters will be consistent with theme styling and there will be less for every theme to override. I've opened a follow-up issue at #2871342: Add gutter styling in Bartik for new layouts to add gutters to Bartik for these layouts.

I've updated the patch to rename Left and Right regions to support RTL sites properly in the future by renaming the regions to their order (First, Second, First Above, etc.) instead of location. I think this is the last change we need.

Makes sense @DyanneNova, working on the failing test.

Let's see what the bot says...

OK this is ready to be RTBCed from my point of view.

It'd be great to have one of the frontend framework managers review this!

Hi Folks,

Just want to leave some screenshots here as I don't have time to work on this just now (but might get an hour this afternoon).

In the 25/50/25 display, I have set the body field and the image field to only two regions (in the first/second/third row) in each screenshot. The results are not consistent.

body second, image third - both take up 50% of the space. Looks great :-)
body first, image second - looks like together they only take up 50% in total. I think it would be better if they both took up 50% each
body first, image third - looks like together they only take up 50% in total. I think it would be better if they both took up 50% each

I'll leave the status as RTBC and leave it with someone with more experience to decide if it should be set back to Needs Work.

Apologies, after further testing, I realised the correct space is being taken up 25% + 25% and the 50% region is just not being printed because it's empty. (Though I think I'd prefer if the 2 used regions became 50% each if only 2 regions are have content in them for that node.) Eitherway, still it's RTBC.

Hello, it's me again!!!

I've had some thoughts about this approach and wish to make a change to it, so that regions fill the available space of the node width. I know this means percentages will change depending on whether there is content or not, but this is something that already happens.

In core, if we have a theme like Bartik with 2 sidebars, each of 25% width and a main content region of 50%, and one of the sidebars has no content, the main content area becomes 75%. So, widths change. We do not leave a white space where the sidebar used to be.

In our present layout CSS if a screen is less than 40em, the regions are all 100%. So, widths change.

My suggestion is two fold:
1) If there is no content in a region, that region is not printed, but the other regions expand to fill its space - using the same proportions that they have with each other. So, if we have a layout of 25/50/25 and only the first 25 and the 50 have content, then we let them expand at a ration of 1:2 (25:50). I'm attaching a patch here to shows how this works - I'm only attaching a patch for this one layout so we can have a discussion about it. If we agree to it, we can do the same for the other layouts, if not, then very little time is wasted.

2) Because of the above, if only the first 25 and 50 regions have content, the width of these in a 1:2 ratio will be 33% and 66%. This is not reflective of the name of the layout "3 Columns 25/50/25). So, the second part of my proposal is to rename the layouts into units. For example, this one could be called "3 Columns (1 unit/2 units/1 unit)". What this means is that if the 3 regions have content, they will have a ratio of 1:2:1 if the first two regions have content they will have a ratio of 1:2, if regions 1 and 3 have content they will have a ratio of 1:1.

We might need to refine the namings of things mention in 2) above, but I think it makes more semantic sense if we decide that we don't want to have whitespace in empty regions.

I am also attaching some screenshots of what happens currently vs what happens with my changes applied.

As I say, this is just a proof of concept for one layout, but should be very easy to apply to other layouts (thanks to the approach already taken by DyanneNova).

Thanks @markconroy for that. I think this would make the layouts more resilient, and keeping the ratios would stay in line with what the users expect from the layout, so +1 from me to this approach.

@markconroy I'm a bit concerned with "widths change" because if you ask for a certain layout 1:2 ratio for example and because one page doesn't have a left sidebar content it expands to fill the full width it's no longer representative of that ratio and could be jarring for a user. Or am I missing something with the proposal you are suggesting?

Hi @joelpittet,

If only one region (of the three) had content in it it would fill 1 unit (and in this case 1 unit would equate to 100%, since there is nothing to divide it by - which is what we do on mobile viewports at the moment). At the moment we have a layout called 25/50/25, but on mobile viewports it's 100/100/100.

The other option is to leave a white space where the region should have been (which might leave a space between first and third (since second has no content) or collapse "second" region and put first and second beside each other with a space on the third which seems counter intuitive (since you have content in first and third).

My proposal says - let's take "mobile-first" and work with it, after that let's be responsive - fill the available space.

When we have a layout like 25/50/25, the 50 may be used on a larger screen to manage the line length to a reasonable readable size, the whitespace is not a bad thing, it can be a visual resting space.

I'm thinking leaving the ratios be those ratios provides some reasonable expectation of how your content will be represented. Maybe I'm behind on the times a bit here?

Maybe you can do a jsfiddle.net to show how this would work with some dummy content?

Hi Joel,

Have you looked at the screenshots in #45 to see how it looks with dummy content.

@joelpittet I've made a fiddle with the CSS and markup for the threecol layout which is the one that @markconroy was showcasing for his proposal, you can find it here:
https://jsfiddle.net/dv5pwrdc/

Keep in mind that if there is no content on one region, then no markup would be produced by Drupal with this patch, so if you want to test the layout in the fiddle you'll have to cut and paste the whole region (including its div) to see how the layout behaves with or without the region.

Hi Manuel Garcia,

Thanks for creating that fiddle. I've forked it and added some extra divs just so we can see all the options for that layout in one place. Also added some inline colours so it's a bit easier to see.

https://jsfiddle.net/pbwzq9ca/

Just adding a screenshot here of the fiddle. Apologies for not adding it to the last comment.

Thank you @Manuel Garcia and @DyanneNova for pointing out this issue for me.

The behavior expected from a layout depends on many things. In some case preserving space for empty columns makes sense and in some cases not. There is no silver bullet that would solve all the use cases. Therefore I suggest that we go with the simple option where empty columns preserve their space.

It might make sense to add more complex layouts as an example to Bartik or the new theme being added to core. Those are both marked as internal which allows changing layouts in them later, in case someone figures out that we guessed wrong on what is the best default behavior.

We should also invest into the documentation how people can create their own layouts so that as many people as possible could create their own layouts, therefore being able to support an infinite number of scenarios.

I would agree with keeping base layouts simple, especially with labels like 'Three column 25/50/25' and 'Three column 33/34/33' I would not expect them to be changing ratios.

re #56 the ratios on the proposal in #45 are preserved, what is different is that we take up the space left by an empty column, so a 25 50 25 becomes a 33.5 66.5 if the last column is empty.

Re #55 Leaving empty spaces for empty columns is how traditionally the panels layouts have worked as well, so I'm guessing people would expect that to be the case still. While I can see benefits to the proposed solution on #56, I can leave without it, and we can of course offer these as contrib, in Bartik, etc.

@tim.plunkett: we can go with the patch on #40 which has the layouts preserving empty columns, and was RTBCed before. Hiding patch #45 for clarity.

I'm re-uploading the patch from #40 to be clear on which patch represents the latest discussions.

Good work everyone! The markup already looks a lot cleaner in the layouts.

1. +++ b/core/modules/layout_discovery/layouts/threecol_33_34_33/threecol_33_34_33.css
   --- a/core/modules/layout_discovery/layouts/twocol/layout--twocol.html.twig
   +++ b/core/modules/layout_discovery/layouts/twocol/layout--twocol.html.twig
   
   +++ b/core/modules/layout_discovery/layouts/twocol/twocol.css
   --- a/core/modules/layout_discovery/layouts/twocol_bricks/layout--twocol-bricks.html.twig
   +++ b/core/modules/layout_discovery/layouts/twocol_bricks/layout--twocol-bricks.html.twig
   

   What's the benefit of having two different layouts with two columns? It seems like everything that can be done with the simpler version, could be also done using the more complex one.

2. +++ b/core/modules/layout_discovery/layouts/threecol_25_50_25/layout--threecol-25-50-25.html.twig
   @@ -14,34 +14,40 @@
   +    'layout--threecol',
   

   If we use a modifier of layout, the layout should be included as well.

3. +++ b/core/modules/layout_discovery/layouts/threecol_25_50_25/layout--threecol-25-50-25.html.twig
   @@ -14,34 +14,40 @@
   +      <div class="layout-region layout-region--top">
   

   Is there any reason why this is not an element of the layout component? That would help us even further simplify our CSS.

4. +++ b/core/modules/layout_discovery/layouts/twocol_bricks/twocol_bricks.css
   @@ -1,13 +1,17 @@
   +    flex: 0 1 50%;
   

   IE 9 doesn't support flex and we still officially support IE 9 until #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x has been solved. I do agree that using flex is a good idea but if that issue doesn't get solved, we might have to rewrite these bits before the CSS can be marked stable. This necessarily doesn't require any actions other than making sure the referenced issue gets done.

5. +++ b/core/modules/layout_discovery/tests/src/Kernel/LayoutTest.php
   @@ -135,11 +135,11 @@ public function renderLayoutData() {
        $data['layout_onecol'][] = <<<'EOD'
   ...
        $data['layout_test_1col_with_form'][] = <<<'EOD'
   
   @@ -165,14 +165,14 @@ public function renderLayoutData() {
        $data['layout_test_1col_no_template'][] = <<<'EOD'
   

   This is not in the scope of this issue but we really should open a follow-up to remove this type of one on one HTML tests since they are really expensive to maintain. What we really care is the structure of the HTML, not all the spaces & other things included in this type of tests.

#63.4
I agree, flexbox use is some concern.. not just because of "older" IE's.

PS: ie 10 isn't really that old! and certainly will have a wide user base for some time. For those who remember IE6 support issues

http://caniuse.com/#feat=flexbox

Even slightly older safari's chrome's and firefox's don't support it.

Perhaps it would be better to hold off on flexbox based css in core for this reason. Certainly Contrib and custom mod's used in real world projects can define their own support scope rather that core introducing a technique that is so modern.

Perhaps it would be better to hold off on flexbox based css in core for this reason.

Most open source software anymore develops against the latest of anything; especially in regards to web.

I think core should get out of the business of trying to support outdated browsers period.

If sites/projects need to support older browsers, then I think it's their responsibility to override for these older use cases.

In regards to flexbox, it has ~ 97.71% global support (including partial). I think that is more than sufficient.

Most... great!

However..we simply can cover ALL now. a simple media query is a more solid solution in ALL use cases.

one more thought: use modenizer/polyfill to detect flexbox compatibly with media query fallback.

Thank you @lauriii for the review!

Re #63:
1. While its true that layout_twocol_bricks provides a similar layout than layout_twocol, it also provides additional regions below the first set, so i guess it does provide value there. That said, one could argue against it. This issue is to clean up the layouts currently in place, lets keep this issue in scope and discuss whether or not we should add/remove layouts to core in a different issue perhaps.

2. Fixed. Also followed the same pattern for layout--threecol-33-34-33.html.twig

3. I'm not sure what you mean here :)

4. Let's work on that issue then. I have added comments about this to each of the CSS files about this, we've got time before this is marked as stable to get #2842298: [policy, no patch] Drop IE9 and IE10 support from Drupal 8.4.x in.

5. Agreed. What should we name this follow up issue? I'm afraid this is currently not in my comfort zone...

Also added @file comments to all CSS files to conform with CSS coding standards.

Thanks @lauriii for kindly explaining point #63.3 to me. Addressing that in this patch.

There is one template which I have doubts on, core/modules/layout_discovery/templates/layout.html.twig any further changes we should be making to that one?

This should hopefully fix all failing tests.

I forgot to post my thoughts from Slack to here, but @Manuel Garcia was right on #69 that we have to also make the same changes to core/modules/layout_discovery/templates/layout.html.twig .

Tackling layout.html.twig:

• Now checks if there's content to be printed in the region before printing the region's markup like on the rest of template files.
• Includes the same CSS class pattern: layout__region layout__region--NAME

While doing this it occurred to me: If we were to use layout.html.twig for all core layouts, it would now produce exactly the same markup as the template files we are cleaning up in this patch, which begs the question: Do we really need these? We could just as well remove them and have all core layouts use layout.html.twig with different css files... thoughts?

Well, I've spent a good hour trying to fix the failing test, but I've gotten nowhere trying to figure out how the new indentation affects the test... :(

Brilliant @tim.plunkett thanks for that!

OK I think we can go back to RTBC now.

Looks like an unrelated test failure...

Unrelated failures (#2859704: Intermittent segfaults on DrupalCI (some "did not complete due to a fatal error" with no additional info)).

It seems like #63.2 didn't get done yet. That point means we have to always include layout class if a variation of that class is being used, i.e. layout--one-col.

Besides that, there are still a few other things to take a look at:

1. +++ b/core/modules/layout_discovery/layouts/threecol_25_50_25/threecol_25_50_25.css
   @@ -1,17 +1,26 @@
   + * Using display: flex requires https://www.drupal.org/node/2842298 to be in
   

   We should add a @todo here to make it easier to find these comments.

2. +++ b/core/modules/layout_discovery/templates/layout.html.twig
   @@ -4,15 +4,19 @@
   +  <div{{ attributes.addClass(classes) }}>
   +  {% for region in layout.getRegionNames %}
   

   The for tag is not intended correctly (missing 2 spaces)

Addressing #86.1 and #86.2.

RE: what still needs to happen regarding #63.2, this is what we've got so far, what are we missing?

• layout--onecol
• layout--twocol
• layout--twocol-bricks
• layout--threecol-25-50-25
• layout--threecol-33-34-33

Addressing #63.2 - Thanks @lauriii for the explanation

Oops thanks @tim.plunkett - fixing that..

Fixing the tests I broke...

This looks good now. Thanks for making all of the changes!

Committed 5184e99 and pushed to 8.4.x. Thanks!

Since this only affects experimental modules, we could also potentially backport this to 8.3.x.

Thanks!

Patch on #95 applies cleanly to 8.3.x, - here it is against 8.3.x just in case.

same patch as #95, came back green, back to rtbc

Committed 4b84859 and pushed to 8.3.x. Thanks!

With this update, regions in the twocol and threecol layouts have changed their names, making content disappear... why no change notice?

@freelock, probably because the module is experimental and so doesn't provide an expectation of BC. However, we can still add a change record to help those using the experimental module. Maybe you can provide one since you already had to work around this problem? Thanks!