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: backdrop/backdrop#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 #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! 😄

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 😛

[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?

[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 backdrop/backdrop#1336. Please make a new one when ready.

[docwilmot]

Personally I like my approach better 😄. 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 backdrop/backdrop#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 backdrop/backdrop#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: backdrop/backdrop#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 #370, so we'll pick up fixing this problem there.