Use grid for layout paragraphs layouts

[markconroy]

Closes #786

What does this change?

• Adds template for 2 col layout
• Adds template for 3 col layout

It might be worth doing the other templates for our page layouts too, but putting this PR here first to get the conversation going.

How to test

• Clear the cache to pick up the new templates
• Check that on homepage (using demo content) the 2 column and 3 column items are using CSS grid instead of Flexbox

How can we measure success?

We have a more unified way of doing grids.

Have we considered potential risks?

This will create a small visual change to some sites, so we need a notification to mention this. E.G. on these layouts by default we have 5px padding each side of the columns on large screens (10px gap in effect), but using our grid system our default gap is 1rem (16px usually), adding in effect 3px extra to the spacing.

---

Thanks to Big Blue Door for sponsoring my time to work on this.

[tonypaulbarker]

“It might be worth doing the other templates for our page layouts too, but putting this PR here first to get the conversation going.”

@markconroy would releasing these sorts of changes together cause less disruption to customisations or little and often smaller updates? I’m genuinely unsure which approach is more helpful to dev teams. Generally I favour smaller changes little and often but this might be a pain to keep coming back to.

[tonypaulbarker]

We could look to do something like this instead and mark flex CSS rules deprecated:

.paragraph--type--page-section .lgd-grid--threecol

We would no longer need lgd-row__one-third as well as the container class in the mark up but lgd-grid-row__full could still span three columns.

[markconroy]

@tonypaulbarker I agree, I think in this case I'd like to do them in one go. I didn't have time this afternoon when doing this PR, but I'll try get it done tomorrow afternoon instead.

[markconroy]

@tonypaulbarker Wanna test this now? I think I have each of the layouts we need catered for now.

[msayoung]

commenting so that I see what happens with this :)

[finnlewis]

@tonypaulbarker wants to do more testing on this one :)

[tonypaulbarker]

@markconroy I explored the workings of these layouts and I think these changes are a good direction to go in for consistency. I did see some breaking layouts on a site, due to some incompatible CSS in the child theme. We should include a warning in the release notes and increment a minor version.

On which templates to support in localgov_base (I thought handy to show all my workings!) :

These are the options I see in the UI:

Columns: 1

• Flexible
• One column

Columns: 2

• Two column
• Two column bricks

Columns: 3

• Three column 25/50/25
• Three column 33/34/33

LocalGov Drupal

• One column (simple)
• Two column (simple)
• Three column 33/34/33 (simple)
• Four column 25/25/25/25 (simple)

I think one column grids should receive the same treatment as multi-column grids - a one column row is still a row.

I think the UI for choosing the layout options available to editors is confusing - e.g. as a site builder how do I make an informed choice to use LocalGov Drupal three column layout instead of the contrib version? What does ‘flexible’ do and under what conditions?

I’m hesitant for us to include layouts for the contrib options in localgov_base. There might be a case for turning those off for new sites if they don’t do anything additionally useful. If they are valuable I think some inline information about the options would help. If we do support any contrib options in localgov_base though, I think we should include templates for them all because 'Three column 25/50/25' should have identical implementation to 'Three column 33/34/33' aside from the column span.

There is also LocalGov Extra Layouts to consider and likely should be included. I note the templates are not prefixed localgov and probably should be: https://github.com/localgovdrupal/localgov_extra_layouts

[markconroy]

I think the UI for choosing the layout options available to editors is confusing - e.g. as a site builder how do I make an informed choice to use LocalGov Drupal three column layout instead of the contrib version? What does ‘flexible’ do and under what conditions?

I agree. I've no idea what the difference is between one column and one column flexible - they have been there since the beginning. But ... I think "what" they do and how we present/explain it should be a follow up issue to this one. Let's just focus on using the new grid system in this issue.

Are we okay to approve this PR? I'm not sure from your comment above if there's more work to do or not.

[tonypaulbarker]

@markconroy

I agree that making the options clearer is for follow up. I think there is more work on what templates to include here. It looks as though we have templates for three of the four LocalGov templates listed and two of the others. I’m not clear why only those two contrib ones were chosen.

What do you think of the following statements?

“I think one column grids should receive the same treatment as multi-column grids - a one column row is still a row.”

“There is also LocalGov Extra Layouts to consider and likely should be included.”

“I’m hesitant for us to include layouts for the contrib template options in localgov_base.”

“If we do support any contrib options in localgov_base though, I think we should include templates for them all because 'Three column 25/50/25' should have identical implementation to 'Three column 33/34/33' aside from the column span.”

[markconroy]

@tonypaulbarker I've templates added now for the one col layouts, and also moved the templates into corresponding directories (one-col, two-col, three-col, four-col).

[tonypaulbarker]

@markconroy I think 'Flexible' is for a flex layout, say you wanted some smaller components of any width to lay out side by side. But it doesn't seem to actually work on a site I tried. Do we have some classes already we could lean on to accomplish that?

[markconroy]

Yes, I think that's what it's for too, but the CSS for it just targets a class called .row which we don't have in our codebase (unless it's somewhere inside some paragraph type or something, but definitely not in any of our layouts).

Looks like we should definitely deprecate that particular layout.

---

Also, targetting .row is way to general - I think it's just a hangover from when we used bootstrap in a very old theme.

[tonypaulbarker]

Is the flex rule lurking here @markconroy ? https://github.com/localgovdrupal/localgov_subsites/blob/2.x/css/one-column-flexible.css

I think that we should probably just delete the flexible template here so it will be business as usual if someone has done something with the flexible option and make a follow up issue.

[markconroy]

I don't think we can delete the template, because current sites may be using it. But we can delete that flex rule from the subsites module I reckon, and if this template is the same as the other one-column template then we can just delete the contents of this one and say @include /the/other/one-column/template

[finnlewis]

Worth looking at this too: localgovdrupal/localgov_paragraphs#264

We might want to align on naming conventions for these templates and classes.

@tonypaulbarker : how many options do we want? How to avoid confusion?

We declare the layouts in https://github.com/localgovdrupal/localgov_paragraphs/blob/2.x/modules/localgov_paragraphs_layout/localgov_paragraphs_layout.layouts.yml

So we're overriding the layout templates and css here in localgov_base.

Let's try to make this consistent!

[tonypaulbarker]

localgovdrupal/localgov_paragraphs#264 <- some extensive related notes here

[markconroy]

@tonypaulbarker @finnlewis Any thing else we need to do with this PR? I don't think Adnan's changes in localgovdrupal/localgov_paragraphs#264 will affect this, at most it might mean we need to add another CSS class or two to a new template (but that will be simple in a follow up).

I'd LOVE to get it merged as it will help unify our layout system.

[markconroy]

https://www.drupal.org/project/localgov_base/issues/3563268