Problem/Motivation

Similar to templates like views-view-list.html.twig that support attributes for each row, layouts need attributes for each region.
This will allow dynamically specifying attributes, which is necessary for things like drag-n-drop UIs.

Proposed resolution

Add a region_attributes variable to layout templates
Currently, the manual adding of classes like 'layout__region' and 'layout__region--second' is kept in the template, but could now be handled dynamically if so desired.

Noteworthy: Display Suite and Bootstrap Layouts both have implemented their own per-region attribute system, which won't conflict with this change, and theirs could easily move to this approach.

Remaining tasks

N/A

User interface changes

N/A

API changes

Layout templates should make the following change:

-    <div class="layout__region layout__region--content">
+    <div {{ region_attributes.content.addClass('layout__region', 'layout__region--content') }}>

This is not a true API change since the code is experimental.

Data model changes

CommentFileSizeAuthor
#13 2885877-layout-region-attributes-13-interdiff.txt897 bytestim.plunkett
#13 2885877-layout-region-attributes-13.patch16.25 KBtim.plunkett
#9 2885877-layout-region-attributes-9.patch16.44 KBtim.plunkett
#9 2885877-layout-region-attributes-9-interdiff.txt7.44 KBtim.plunkett
#5 2885877-layout-region-attributes-5-interdiff.txt922 bytestim.plunkett
#5 2885877-layout-region-attributes-5.patch13.66 KBtim.plunkett
#2 2885877-layout-2.patch13.49 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2885877-layout-2.patch13.49 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2885877-layout-2.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

The fixes are fine but the test is blocked by #2862683: 'base hook' key prevents template suggestions from working

Might try to come up with another approach for the test to stay unblocked

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2885877-layout-region-attributes-5.patch13.66 KB
2885877-layout-region-attributes-5-interdiff.txt922 bytes

Working around #2886187: Document the regex in Registry::postProcessExtension() used to find preprocess functions for now.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
+++ b/core/modules/layout_discovery/tests/src/Kernel/LayoutTest.php
@@ -167,11 +167,11 @@ public function renderLayoutData() {
+    $html[] = '<div data-drupal-selector="edit-left" class="class-added-by-preprocess region-left">';

+++ b/core/modules/system/tests/modules/layout_test/layout_test.module
@@ -0,0 +1,17 @@
+    $variables['region_attributes']['left']->addClass('class-added-by-preprocess');

This is the key part, preprocess adding classes to region markup.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Issue summary: View changes

Updated the IS to mention that this is similar to DS and Bootstrap Layouts.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/layout_discovery/layouts/twocol_bricks/layout--twocol-bricks.html.twig
@@ -22,43 +22,43 @@
-      <div class="layout__region layout__region--first-above">
+      <div {{ region_attributes.first_above.addClass('layout__region', 'layout__region--first_above') }}>

    first-above 

  2. +++ b/core/modules/layout_discovery/layouts/twocol_bricks/layout--twocol-bricks.html.twig
@@ -22,43 +22,43 @@
-      <div class="layout__region layout__region--second-above">
+      <div {{ region_attributes.second.addClass('layout__region', 'layout__region--second') }}>

    second-above 

  3. +++ b/core/modules/system/tests/modules/layout_test/templates/layout-test-1col.html.twig
@@ -4,11 +4,19 @@
+{%
+  set classes = [
+    'layout-example-1col',
+    'clearfix',
+  ]
+%}
+{% if content %}
+<div{{ attributes.addClass(classes) }}>

    Can we do this just as:

    {{ attributes.addClass('layout-example-1col', 'clearfix') }}

    instead?

Tad bit of tweaking and this will look RTBC to me.

Eclipse

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2885877-layout-region-attributes-9-interdiff.txt7.44 KB
2885877-layout-region-attributes-9.patch16.44 KB

1+2) nice catch! Went back over it with diff color-words and found some other mistakes

3) Yes, agreed. I'm keeping that approach in the real templates since it's what the rest of core does. But I minimized the changes to the test layouts.

Additionally, made the same changes to threecol 25/50/25 and the generic layout.html.twig

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented
Status: Needs review » Reviewed & tested by the community

#8 has been addressed, all layouts templates covered, this looks great to me, rtbcing

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

RTBC++

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Reviewed & tested by the community » Needs review
FileSize
2885877-layout-region-attributes-13.patch16.25 KB
2885877-layout-region-attributes-13-interdiff.txt897 bytes

@effulgentsia points out that we don't even need the @todo pointing to #2886187: Document the regex in Registry::postProcessExtension() used to find preprocess functions, or that issue at all, because the simpler template_preprocess_layout_test_2col already works.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Reviewed & tested by the community

RTBC pending testbot

Eclipse

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext for University of Technology, Sydney commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Can we have a change record for this.

I realize that layout discovery is experimental, but the fact is anyone using DS on 8.3 is most likely to be using it.

And they are most likely to have their own layouts, so will need to make similar updates.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Added https://www.drupal.org/node/2886860

WidgetsBurritos’s picture

WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commented
Status: Needs review » Reviewed & tested by the community

+1 Change record looks good to me.

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented
Status: Reviewed & tested by the community » Needs work

change record: s/row_attributes/region_attributes ?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Reviewed & tested by the community

Changed.

Anyone can edit CRs, could have just fixed it :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Updating issue credit and title

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Title: Layout templates need support for per-region attributes » Add support for per-region attributes to Layout Templates
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

I reviewed this patch and applied it locally.

I checked both of the .layouts.yml in core (one in layout_discovery.module and one in layout_test.module) and made sure that each of the defined layout templates were updated to use the new attributes - all were.

Committed as 7996c0f and pushed to 8.4.x.

Published the change record.

Thanks for your work.

  • larowlan committed 7996c0f on 8.4.x 
    Issue #2885877 by tim.plunkett, EclipseGc: Add support for per-region...
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper at PreviousNext commented

Congrats on your first core commit, larowlan!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.