The highlighted region in Bartik is missing layout styles.

RTBC from me but assigning to emma for a second look.

Thanks @Dom.

We somehow missed this during #2289917: Convert "messages" page element into blocks. I've also seen this happen.

-1 because this adds a new preprocess hook, and is adding a class. Shouldn't this be fixed by a change in the CSS instead?

Putting back to NR, I want to prevent a commit before @emma.maria has had a chance to look at this.

@Wim Leers, putting a new preprocess hook isn't too bad on bartik, it is if it's in core as we are moving these to templates. We could add that class into a duplicated region.html.twig template but I prefer the simplest approach and I think @Dom. found it.

We can also do it by adding the width/margin to the element directly to the existing classes, but that would be duplicating the purpose of that utility CSS class. With SASS we could make that a mixin but I don't think it makes sense in this case to put the CSS duplicated from the utility class.

I'm no template system/best practices expert, so my apologies if I derailed this. It just looked quite odd.

Wouldn't it be cleaner to have a region--messages.html.twig whose entire contents looked like this:

{% extends "region.html.twig" %}
{% set attributes = attributes.addClass('layout-container') %}

i.e. much like core/modules/text/templates/field--text.html.twig.

No worries, there are lots of ways to accomplish this task. Choosing the 'right' way is nuanced.

I'm fine with the region method, either globally or that messages region suggestion as you pointed out in #7.

That is likely the correct way to go for the Banana Initiative:P From maintenance of template files it's a bit of a pain IMO, but maybe we can @davidhernandez or @mortendk's opinion or just go with whatever @emma.maria suggests for the way they want to push this theme towards in terms of consistency.

The proposed solution above does copy the method used for other classes added as you can see in the patch's hunk context. But that may not be the way they are aiming for.

#splittinghairs

subscribing this issue...

#8: yeah, I first thought that this should be fixed with CSS, which is why I un-RTBC'd it. OTOH, it's also kind of important to set a precedent: do we want themes to add classes using preprocess hooks or using #7. And indeed, #7 is aligned with Banana Initiative.

#9: there's a big "follow" button ;)

My opinion is that the core policy of not having classes in preprocess does not need to extend to Bartik and Seven. They should strive for best practices, but should be treated as almost independent projects, and mostly* do what is best for them. (ie. what the maintainer wants.)

The only use-case where Bartik having something in preprocess is a problem, is if someone subthemes it. Frankly, they should copy it instead. It is not designed to be a base theme.

So in this case I would do what is simplest/makes the most sense. If the template already exists #7 would make sense, but it is probably not worth adding a new template file just for this. Ultimately, I would leave that up to Emma. If she wants to add the template file, go ahead; otherwise, preprocess is fine.

I introduced this regression in #2398451: Clean up "layout" CSS in Bartik (sorry). I uploaded a patch the next day to fix the regression and clean up the messages component in Bartik, but its current status is still 'Needs review'. It's available at #2456805: Clean up the "messages" component in Bartik

There is no need to add a preprocess function to add this class by the way. Bartik already has a template file for messages where the class can be added.

Aha! This issue is fixing the messages region. But #2456805: Clean up the "messages" component in Bartik fixes it for the messages block. I don't know Bartik's structure & conventions well enough to know which is correct. But depending on the answer to that question, it's either this issue, or the other that is the correct solution :)

@Wim Leers When the 'Featured top' has content, the messages block should have a max-width because the background needs to be full-width grey, so the approach in #2 here is incorrect. A screenshot is probably easier to understand:

Screenshot of the implementation in #2:

Alright, seems pretty clear to me. I'll leave it to a stronger front-end person to close this issue in favor of the other though.

I am going to take a look at #2456805: Clean up the "messages" component in Bartik right now and see if that issue fixes the problem and report back here.

I would like the messages region to have the same layout and styling format as other sections of Bartik.
Now that messages are a block that sit in a region there is no reason to not treat it as so.

To follow the mark up formats of other regions the markup for the Messages region should be...

<div class="messages">
  <aside class="layout-container section clearfix" role="complementary">
     <div class="region region-messages">
       content of messages block

This can be set in the page.html.twig along with an if statement like the other regions. We do not need to preprocess or add another template.

However the main problem I have is that the messages class is already taken for the messages component. We shouldn't have region names that are tied to the default component set to it, as this problem kicks in when using component based styles. We also currently have a CSS file that contains both the block styling and the region styling called messages.css and that shouldn't be the case.

@Dom. If we place it inside of the featured top region then it could get a grey background behind it all the time from that region.

@Dom. Yeah it looks bad, but mostly we shouldn't be changing the design in these issues :\

I have created this issue to rename the Messages region...

#2470807: Rename the default "Messages" region for all themes to "Highlighted"

Postponing this issue until the region name has been changed and we can use the final markup that we require in Bartik.

Keep an eye on this issue #2470807: Rename the default "Messages" region for all themes to "Highlighted".

This is still postponed but #2470807: Rename the default "Messages" region for all themes to "Highlighted" is RTBC so as the patch is trivial I'll jump the gun. I'm also renaming this issue to match that naming convention.

Attached is a really small patch implementing @emma.maria's proposal from #17. This patch will NOT work on HEAD, because it assumes 2470807 has been committed, so:

1. for now, patch HEAD with rename_the_default-2470807-49.patch first.
2. then apply this patch

I've also attached a screenshot showing the behaviour of the messages block with everything in place.

I know it's hubris, but hopefully once 2470807 is approved this should hopefully be ready to commit too :)

Now that #2470807: Rename the default "Messages" region for all themes to "Highlighted" is fixed we can now fix this bug :)

We need a review of the patch in #26 which uses the new markup.

rerolling a #26 patch and removing message region from bartik.info.yml file.

ohh got it.. forget to pull the latest code after #2470807: Rename the default "Messages" region for all themes to "Highlighted" fixed.. patch #26 is applying successfully ;)

I changed the markup since the one from #26 appears to be wrong. However, atm it wont work because this issue here #2470807: Rename the default "Messages" region for all themes to "Highlighted" got reverted and needs work.

Okay, so heres my patch, but because of this #2470807: Rename the default "Messages" region for all themes to "Highlighted" it’ll probably need to be postponed.

+++ b/core/themes/bartik/templates/page.html.twig
@@ -109,7 +109,15 @@
+    <div class="highlighted">
+      <aside class="highlighted__inner section layout-container clearfix" role="complementary">
+        {{ page.highlighted }}
+      </aside>
+    </div>

Just needs to be indented once more inside the if statement.

Emma does this approach suit the needs for this issue?

Indented as suggested by @joelpittet.

I think at this point we just need to decide which markup is preferred. Here are before & after screenshots showing #26 and #36.

If the markup from #26 seems better (and it's definitely closer to @emma.maria's proposed markup in #17) I think it just needs to be wrapped in an if block like #36 is.

Before:

After (#26):

After (#36):

Both #26 and #36 suffer from an implementation issue I explained in #14

The solution in #40 results in a change in background-color for the highlighted region depending if there are messages on the page.

#26 is not quite correct it prints the region div which is printed within the variable {{ page.highlighted }}.

#36 contains the correct levels of markup, classes etc.
 

 
Looking at it again, I am not 100% sure if we should be using an tag within the Highlighted region. I assume that is used for content that is secondary related content to the main content. Highlighted by default is used for information about the site and not the main content on the page. Can someone clarify this for me? If we shouldn't use can it can it please be changed to a straightforward

.has-featured-top #messages {
  background: #f0f0f0;
  background: rgba(30, 50, 10, 0.08);
}

@Dom - The patch in #40 did not exist at the time I was reviewing the issue. So disregard my comment. The latest patch I was looking at was #36 and we cross posted. I will review your patch in #40.

@Dom No worries, thanks for the patch! :)

Trying to fire testbot on the patch in #40.

Re-uploading the patch from #40 with no changes. I cannot for the life of me get testbot to acknowledge it.

The messages now print in the region Highlighted using the correct markup for the new region name and now finally show at the correct width.
 

 
and with the Featured Top region.
 

 

As this issue is to visually fix the messages I deem this RTBC. Any follow up issues with the code (if found) can be dealt within this issue... #2456805: Clean up the "messages" component in Bartik.

Great work everyone!

Recommended commit message:

git commit -m 'Issue #2462221 by emma.maria, Dom., rudraram, jp.stacey, Manjit.Singh, sxnc, Cottser, idebr: Highlighted region has not fixed width layout on Bartik theme'

Sneaking in the missing Beta evaluation.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed f03eba7 and pushed to 8.0.x. Thanks!