Add class to layout region.

[Gormartsen]

Add ability to add class to layout via template.php Right now class hard coded to layout tpl file.

It will help to support different layouts on theme level.

Latest pull request: https://github.com/backdrop/backdrop/pull/1356

[docwilmot]

there's already a $classes array in each layout template which can be modified in a hook_preprocess_layout__layout_name() function in your theme. See https://github.com/backdrop/backdrop/blob/1.x/core/layouts/three_three_four_column/three_three_four_column.php for example.

Would this be what you need, or are you looking for something more specific?

[Gormartsen]

Thank you @docwilmot .

See this line https://github.com/backdrop/backdrop/blob/1.x/core/layouts/three_three_four_column/layout--three-three-four-column.tpl.php#L41

<header class="l-header" role="banner" aria-label="<?php print t('Site header'); ?>">

[Gormartsen]

For example, I am porting bootstrap theme. I need to add "well" class to spec layout region.

Right now I need to basically rewrite all available layout templates to add class to specific region. And when new layout will be introduced, I need to update theme to support new layout as well.

Does it make sense now?

[docwilmot]

You mean like <header class="l-header <?php $header_class ?>" role="banner" ...? Thats going to be difficult to maintain. We'd need a region class variable for every single region, and all contrib devs would have to do the same for every region in their contrib layouts. Themes don't do that either.

You could add a gormartsen class to the $classes array, and then do .gormartsen .l-header {} in CSS and copy the definitions from .well to that. That gets you halfway at least.

[Gormartsen]

I see your point. On another hand, theme developer need to update theme each time when layout template has been changed to support new features. Or add layout support every time when new layout has been introduced.

If I boil it down, there is two options:

• all theme developers spend extra time every time to support new or updated layout
• all layout developers spend extra time once to add class flexibility to regions.

I have other way to add well class to region:

backdrop_add_js('(function($){ $(".l-' . $region_name . '").addClass("' . $well . '");})(jQuery);', array('type' => 'inline', 'scope' => 'footer'));

[quicksketch]

I've added this issue to the Layout Followup Meta at https://github.com/backdrop/backdrop-issues/issues/345.

Thanks @Gormartsen for opening this issue. It's come up a couple of times, especially with front-end developers. @wesruv mentioned it when he first started with Backdrop.

So the basic reasons why classes are not supported:

• The main reason why classes have been desired on regions is to make it so that themes can control the site layout. For example by adding a grid system class. However, Layouts intentionally are attempting to decouple from Themes. If the classes are provided by a theme, then you're tying the layout to theme again.
• We don't have the ability to add settings to regions (yet), so this is on the slightly more difficult side.

However, the first reason is mostly philosophical. There are other legitimate reasons why you may want a class on a region. And if people do want to use classes for layout-purposes... well that may be fine too, even if it breaks our attempt at separating layouts and themes.

So to make this possible, we'll need to incorporate some way of indicating which layouts support region classes (as right now, none of them do). I think something like this in the layout .info file may pave the way for this and other abilities:

features[] = region_classes

Similar to how we used to have features[] for themes in D7. In Backdrop, we don't have these any more as the "features" were all moved into different blocks.

Anyway, the flag in the .info file would enable the ability to set a class in the UI when a layout supported it. The core layouts should then be updated to support region classes in their template and .info files.

[wesruv]

Want! :smile:

My sample use case for why we should have this is: A sidebar should be structurally defined in the layout If I want to give the sidebar a background color, it should be done in the theme But there isn't a reliable way to do it currently

Layouts could have different names for the region, or maybe one layout doesn't have a sidebar, per se.

I'd also like to see a way for themes (and maybe layouts) to define custom classes so a site builder could select them from the UI, instead of relying on copy/paste, spelling, etc. But that'd be trickier :stuck_out_tongue:

[docwilmot]

I'd also like to see a way for themes (and maybe layouts) to define custom classes so a site builder could select them from the UI

@wesruv could you explain this a bit please? Trying to get what you mean.

The core layouts should then be updated to support region classes in their template and .info files

Are we talking about doing this (or similar) after all? <header class="l-header <?php $classes['header'] ?>" role="banner"...> If so could we start by adding a matching key to the $classes array for each of the region variables, just like above. Then a user could at least use a preprocess function. Then we could work on a UI later?

features[] = region_classes

Why this? Why not just make this standard for all layouts?

[Gormartsen]

Personally, I would love to be able to add class via preprocessor.

Then I can add UI to theme-settings to colour regions for specified layouts or all layouts.

For example add class "fancy-background" to all layouts that support sidebar.

Then enduser can select to add fancy-background to only selected layouts and use them on specified pages.

Looks promising to me.

[docwilmot]

Thought I'd give this a go; it turned into a full-blown foray into Region Styles rather than just adding classes. Leaving it here for review; tests will fail.

[docwilmot]

P.s. haven't touched CSS so dropbutton looks wonky.

If only one style is available

Select for multiple styles available

[klonos]

Why does the dialog still have "Add block" instead of "Configure [region_name] region" as title?

[klonos]

...also just noticed that the label for the global layout title type drop-down menu (the one right below the "Edit layout" tab) is "Block title type". Shouldn't that be something like "Page title type" instead? ...in all the screenshots in #934 for example, I see it as "Title type". Some accidental thing with a recent commit perhaps?

[wesruv]

Other than the little snags mentioned above, I dig the direction! I'll try to give the code a whirl too

[Gormartsen]

Tnx guys!

[quicksketch]

Awesome work @docwilmot! You totally took the half-finished work on "region styles" and made it work again! That came from panels in D7 but was never finished. Excellent work!

I tried out the PR and ran into a few of the same issues noted above.

However, caveats aside, this works!

I also love the direction. Just add a child wrapper and keep the entire business of classes out of the layout template files.

[quicksketch]

One downside to this approach: People may expect that by adding a class to the "region" that they would be able to use layout-controlling classes, like Twitter Bootstrap's column system. Because this is a wrapper inside the layout region div, these classes won't be able to affect the layout itself. It's both a good and bad thing IMO. With this approach, are we really giving people what they want?

[docwilmot]

I noticed this as well. Earlier in https://github.com/backdrop/backdrop-issues/issues/1300#issuecomment-146612153 I suggested <header class="l-header <?php $classes['header'] ?>" role="banner"...> in layout templates. This would allow manipulating layout classes manually.

An idea then if we implemented the above: We could also by default copy the region class to this new $classes array with an l- prefix. So if I add a region style of class myclass, the inner wrapper gets this class as with this PR, but the $classes array also gets an l-myclass automatically. `

[Gormartsen]

@docwilmot thank you!

I created this issue to avoid situation like this:

https://github.com/backdrop-contrib/bootstrap_lite/blob/1.x-1.x/template.php#L99

backdrop_add_js('(function($){ $(".layout").addClass("' . theme_get_setting('bootstrap_lite_container') . '");})(jQuery);', array('type' => 'inline', 'scope' => 'footer'));

See attached screenshot as well:

Basically in this case it required to make layout a container-fluid (100% width) or container limited width.

It's from bootstrap:

@media (min-width:768px) {
    .container {
        width: 750px
    }
}

@media (min-width:992px) {
    .container {
        width: 970px
    }
}

@media (min-width:1200px) {
    .container {
        width: 1170px
    }
}

You can see example here: http://bootstrap.backdrop.expert and here: http://bootstrap.backdrop.expert/contact

Instead of adding class to .layout via backdrop_add_js it would be properly to add via hook.

This class does not brake layouts at all. User can still use two column layout, three columns and etc.

Using this hook, theme can give user ability to use all available layouts without braking them, but adding requested feature (fluid or fixed width based on media)

[quicksketch]

In response to above from @docwilmot:

An idea then if we implemented the above: We could also by default copy the region class to this new $classes array with an l- prefix. So if I add a region style of class myclass, the inner wrapper gets this class as with this PR, but the $classes array also gets an l-myclass automatically.

We still wouldn't be giving users the ability to use a grid system that had static class names on the regions. If I were using Bootstrap, I would need the class col-xs-4 for example. If we add that class to the inner wrapper, it's not going to work. And if we prefix with l- to make l-col-xs-4 on the outer wrapper, the CSS isn't going match and won't work either.

So we're still in the same position as before, this adds useful functionality but it wouldn't allow for (out-of-box) combining of existing layouts and grid-system class names. I'm not sure that's a goal we should pursue anyway, there are a lot of other challenges besides just adding a class to the region. For example you'd also need to add a class to the overall page wrapper to set the number of columns desired for the page, and you'd need to group each collection (row) of columns in .container or .container-fluid.

But overall though, this still seems like worth adding as-is. There are a lot of great benefits that could be utilized for site-builders.

[docwilmot]

PR up and tests passing.

[jenlampton]

The request above came from a use-case that we do not recommend (or support). The layout UI should not be used to change the way a layout works. It should not be used to attach a grid-system to a layout that was not meant to have one. That kind of change should be done by creating a new layout - one that has the markup that's needed by the grid system (both HTML structure and classes) - and that includes only the style sheets that are needed (grid system OR otherwise, not both).

Adding a UI that allows people to add another inner-wrapper with a specific class seems handy - and there are likely other use-cases we do want to support (like changing the background-color, or centering all the text) but I dislike the approach of adding in a new template file.

1) If we add the "configure your markup" feature along with a tool that allows a front-end developer to override that same markup, that template will break the UI we are adding, creating additional USABILITY problems.

2) Adding a new theme function / template file also increases complexity, and will make the theme system (and Layout system) harder for people to learn how to use. We specifically removed the theme_region() function after we forked from Drupal in an effort to clean up the theme layer. I would like to avoid adding it back. LEARNABILITY

3) Adding a new template file that may need to be loaded and used on every region on a site also has performance implications (though granted, not much). PERFORMANCE

These issues touch on Backdrop's principles, and we should be careful that we are not introducing new code that goes against several of them, especially if this is a minority use-case. (tbd?)

If we want to add this feature, I'd recommend going about it a different way. If we throw away the approach that was done in panels and re-think this from a simpler perspective, there is a simpler solution that solves all 3 issues above:

Let's just concatenate the inner-wrapper element around the content of the region! I have a PR in the works, working on getting tests passing.

[quicksketch]

The PR at https://github.com/backdrop/backdrop/issues/1336 looks pretty good. However rather than using the style system as it was intended, it's hard-coding the rendering of the region.

We have this bit of code in layout_layout_style_info():

+  $info['region'] = array(
+    'title' => t('Region'),
+    'description' => t('Style for regions.'),
+    'block theme' => NULL,
+    'class' => 'LayoutStyleRegion',
+  );

Where 'block theme' is NULL (that's fine) but it should also have had 'region theme' specified. Then we had existing code to use this theme region:

-    if ($style['region theme']) {
-      $output = theme($style['region theme'], array('layout' => $this->layout, 'blocks' => $blocks, 'settings' => $style_settings, 'style' => $style));
-    }
-    else {
-      $output = implode('', $blocks);
+    $output = implode('', $blocks);
+
+    if (isset($this->prepared['regions'][$region_id]['style settings']['element'])
+      && ($this->prepared['regions'][$region_id]['style settings']['element'] != '')) {
+      $wrapper_tag = $this->prepared['regions'][$region_id]['style settings']['element'];
+      $classes = $this->prepared['regions'][$region_id]['style settings']['classes'];
+      $output  = '<' . $wrapper_tag . ' class="' . $classes .'">' . $output . '</' . $wrapper_tag . '>';
     }

Now what is happening is that style information in hook_layout_style_info() is not checked at all on upon rendering, the style settings are hard-coded, and no template is used. I know @jenlampton intentionally avoided adding a template file here, but this set of changes makes for a crufty approach where we have a system that we're not actually using. We knew that we weren't using it previously in the initial port, but this would be an opportunity to restore what we had ported from Panels.

I like the new options for optionally creating an inner wrapper, but implementation-wise I think https://github.com/backdrop/backdrop/issues/1256 was a better approach.

So, options I think we have here:

• Restored the previous approach, which will use the information from hook_layout_info() and re-add the template.
• Make a view() method within the LayoutStyle base class and the LayoutStyleRegionsubclass that is used to actually do the rendering. Then the settings and the rendering is all kept together in the same place (and the layout-region.tpl.php file could be avoided).
• Remove the concept of region styles entirely and hard-code the whole thing. Then we wouldn't have hook_layout_style_info() for regions.
• Leave things as is, which breaks the previously swappable system to one that isn't swappable at all, but still requires registration though hook_layout_style_info() even though you can't add/remove/change the styles used for regions.

[quicksketch]

Whichever approach we use, we should update hook_layout_style_info()'s documentation. In the previous PR, the existing documentation was actually made accurate, because it started using the system that was already in place. Any other approach we should update it to reflect how hook_layout_style_info() now works and whether it can be used on regions.

[jenlampton]

we should updated hook_layout_style_info()'s documentation

I took a stab at updating the docs in this latest PR. I removed all mention of layout theme since that is not used, and corrected path to template since that was previously incorrect.

I like either option #2 or #3. I don't think we need the system to be swappable here. I've used the heck out of Panels and never needed to swap it, so I'm leaning towards #3. We'd still have the inner wrapper, just no "styles". 👍

[quicksketch]

I'm fine either way, I just don't like carrying around abstracted code if we're not going to be using it in the first place. Personally, I'm still of the opinion that the regions should be themeable, and so I like @docwilmot's original approach best (though the added options in @jenlampton's later version was definitely an improvement as well).

I've closed the PR at https://github.com/backdrop/backdrop/pull/1336. Please make a new one when ready.

[docwilmot]

Personally I like my approach better :smile:. I think the ability to theme the region is a good thing and a template to do so is useful. Also I'm not so excited about an additional wrapper around the region content and that aside and section should be elements provided by the layout (why have an entire region marked as "aside"?) So FWIW https://github.com/backdrop/backdrop/issues/1256 is still open and passing tests.

[jenlampton]

Also I'm not so excited about an additional wrapper around the region content

In the case of this feature request, "adding a class to a region" means adding a second div tag inside the region for the class to be placed onto. We don't want the region's wrapper tag (or classes on it) to be changed - that might break the layout. If the layout needs to be changed, the layout template is there for that purpose. We already have a tool for that job.

I just don't like carrying around abstracted code if we're not going to be using it in the first place.

I 100% agree, but that abstracted code came from the port of panels. If it's not needed then we should get rid of it. I just didn't know if that was too much change for 1.x.

I'm still of the opinion that the regions should be themeable.

We're talking about a single div tag here. If every individual div tag in our output was themeable we'd end up in a huge mess!

I think the ability to theme the region is a good thing and a template to do so is useful.

Yes, there are a lot of useful things we could add to backdrop, but we need to be careful how many and which ones. These are the kinds of decisions that make Drupal/Backdrop hard for HTML/CSS people to learn. There are already far too many places to change the HTML. We need fewer templates with more variables in them. That means getting rid of template files that provide only a single div tag (and pretty please - not adding new ones?) :)

However, if we need that div tag to be customizable then fine, we bury a theme function in there (not a template file?), but something like theme_region_inner_wrapper() that returns this extra div tag - but how about only when the option to add a class to the region is used?

I'll take a stab at a version with a theme_region_inner_wrapper() and see how I feel about that when it's done.

[jenlampton]

Okay, here's a version with the region styles abstraction removed, but a theme function added. This will satisfy my desire for 1) less abstraction 2) no template files that simply output a div. It does not satisfy my desire to avoid broken UIs. However, since we are allowing HTML to be specified in other UIs in core (and also provide theme devs ways to break those UIs) this is at least consistent :)

I'm hoping it will also satisfy @quicksketch's desire for

• no abstraction where not actually used

I'm hoping it will also satisfy @docwilmot's desire for

• ability for a theme dev to override the region

[docwilmot]

Where is it?

[jenlampton]

Sorry about that @docwilmot looks like I was pushing to the old branch. Also your comment:

aside and section should be elements provided by the layout (why have an entire region marked as "aside"?)

What HTML elements you recommend here? I'm not a HTML5 expert, but it looked like these were common ones. I'm happy to add/remove others as directed :)

[jenlampton]

Looking at this first version of the PR, I think I'd also be okay with this theme function being called even when the extra wrapper element is NOT requested. Would that make more sense?

We could do something like this...

  function renderRegion($region_id, $blocks) {
    $style_settings = $this->prepared['regions'][$region_id]['style settings'];
    $tag = isset($style_settings['element']) ? $style_settings['element'] : '';
    $classes = isset($style_settings['classes']) ? $style_settings['classes'] : '';

    return theme('layout_region_inner', array('blocks' => $blocks, 'tag' => $tag, 'classes' => $classes));
  }

and this

function theme_layout_region_inner($variables) {
  $tag = $variables['tag'];
  $classes = $variables['classes'];

  if ($tag != '') {
    return '<' . $tag . ' class="' . $classes .'">' . implode('', $variables['blocks']) . '</' . $tag . '>';
  }
  return implode('', $variables['blocks']);
}

@docwilmot this would allow all regions to be overridden in the theme - things could be done like BR's added between each block. More utility - but still sans abstraction. People who don't need this would never know it was there.

[docwilmot]

OK I'm warming up to this. Simpler indeed, and not sure anyone would really miss swappable styles.

What HTML elements you recommend here?

Personally I think forget the entire choosing a wrapper element business. Its a single inner wrapper; make it a DIV. I think having other grouping elements make sense if you have a bunch of them

<div>
    <section><...>
    <section><...>

makes sense. But this is always going to be the single element, I cannot imagine any benefit from having a different from just a plain DIV.

this theme function being called even when the extra wrapper element is NOT requested

I think I prefer that way as well.

Finally, the point raised by @quicksketch

People may expect that by adding a class to the "region" that they would be able to use layout-controlling classes, like Twitter Bootstrap's column system. Because this is a wrapper inside the layout region div, these classes won't be able to affect the layout itself.

couldnt be address with the Region Styles approach, but now if we're canning that, is this an opportunity to allow this instead?

[jenlampton]

couldnt be addressed with the Region Styles approach, but now if we're canning that, is this an opportunity to allow this instead?

I think the point is that we don't want to allow it. Back in the very beginning when @Gormartsen mentioned that in the issue, we quickly mentioned that behavior was not only unsupported but also not recommended (and I'd go further to say it should be actively discouraged).

From @quicksketch:

The main reason why classes have been desired on regions is to make it so that themes can control the site layout. For example by adding a grid system class. However, Layouts intentionally are attempting to decouple from Themes. If the classes are provided by a theme, then you're tying the layout to theme again.

The main reason you choose a layout is for the layout it provides. If you don't like it, choose another layout. (Or, override the layout template in the theme.) Attempting to alter it using CSS classes in the UI is opening a whole can of worms :)

I am expecting there to be some sort of layout-builder that will come along in contrib and provide some sort of layout-building utility. I think contrib is a great place for that. If it matures there and we get something core-worthy from it, we can put that in. But let's not start by allowing layouts to be adjusted in core.

[quicksketch]

The concept sounds okay to me: Remove region styles but maintain the region theme function. I reviewed the PR at https://github.com/backdrop/backdrop/pull/1356, tests have a new failure, and there are a few items that need addressing.

I'm in favor of allowing the user to choose their tag for the wrapper. As with blocks, most of the time a DIV is the right thing, but it's probably a site builder or themer that needs the classes for a purpose and wants as much control of their markup as possible (matching what we've done for blocks and at almost every layer of views).

This set of changes (with or without region styles) won't have the ability to add classes to region wrappers, as there isn't a variable there to inject a class into in the first place. But like Jen has said above, we're actively discouraging the modification (or breaking) of a layout.

[klonos]

I'm in favor of allowing the user to choose their tag for the wrapper. As with blocks, most of the time a DIV is the right thing, but it's probably a site builder or themer that needs the classes for a purpose and wants as much control of their markup as possible (matching what we've done for blocks and at almost every layer of views).

I'd say I'm in favor of this too.

[jenlampton]

Just pushed @quicksketch 's requested changes to the PR

[quicksketch]

The current PR looks great but has a legitimately failing test: https://github.com/backdrop/backdrop/pull/1356. Could you fix it @jenlampton?

[jenlampton]

Fixed the failing test.

[quicksketch]

I've merged into 1.x for 1.4.0. Thanks @docwilmot and @jenlampton!

This has made a problem with our dropbuttons become more obvious. Note the gap between the region title and the dropbutton itself:

But we already have an issue for this at https://github.com/backdrop/backdrop-issues/issues/370, so we'll pick up fixing this problem there.