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'

Problem/Motivation

Regarding latest devs, message region is not fixed width layout in Bartik theme :

This seems odd UX.

Proposed resolution

Add layout-container class on the region-message to get the same width as the rest of the site :

Remaining tasks

- Write patch
- Manual review

User interface changes

Message region will get the same width as the rest of site.

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because other work to make the Messages a block in Core broke the layout in Bartik
Issue priority Not critical because Messages in the Highlighted region function fine, it's a visual bug.
Unfrozen changes Unfrozen because it only changes CSS and markup
Disruption Non-disruptive it is a visual change to accompany work completed in another issue.

Comments

Dom.’s picture

Title: Message section has not fixed width layout on Bartik theme » Message region has not fixed width layout on Bartik theme
Issue summary: View changes
Dom.’s picture

Status: Active » Needs review
FileSize
656 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,225 pass(es). View
joelpittet’s picture

Assigned: Unassigned » emma.maria
Status: Needs review » Reviewed & tested by the community

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

Thanks @Dom.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

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.

joelpittet’s picture

@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.

Dom.’s picture

@joelpittet: thanks for detailing this. That was indeed my analysis of the situation.
@Wim Leers: layout-container is not a brand new class but the reuse of a class from the theme that has this same purpose to reduce and fix the width. Also creating a new custom template for this region seems odd as the display and markup would not change: just a class to add. This seems more a job for a preprocess method than a new template just for that. Of course, you would be welcome however to detail a better solution for me to implement, I would be happy to learn something new.

Wim Leers’s picture

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.

joelpittet’s picture

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

Manjit.Singh’s picture

subscribing this issue...

Wim Leers’s picture

#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 ;)

davidhernandez’s picture

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.

idebr’s picture

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.

Wim Leers’s picture

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 :)

idebr’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
170.97 KB

@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:

Wim Leers’s picture

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.

emma.maria’s picture

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.

emma.maria’s picture

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.

emma.maria’s picture

Assigned: emma.maria » Unassigned
emma.maria’s picture

Issue tags: +drupaldevdays, +Twig, +CSS, +frontend
Dom.’s picture

@emma.maria regarding #17: Should we renaim the region to "Informations" or something ? (even remove it and assign Message block to Feature Top ?)

LewisNyman’s picture

@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.’s picture

@LewisNyman is it that bad ?

LewisNyman’s picture

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

emma.maria’s picture

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

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

emma.maria’s picture

Status: Needs work » Postponed

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".

jp.stacey’s picture

Title: Message region has not fixed width layout on Bartik theme » Highlighted region has not fixed width layout on Bartik theme
FileSize
750 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,242 pass(es). View
182.61 KB

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 :)

emma.maria’s picture

Status: Postponed » Active
emma.maria’s picture

Status: Active » Needs review

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.

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Manjit.Singh’s picture

FileSize
1.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch fixed-size-highlighted-region-2462221-30.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 30: fixed-size-highlighted-region-2462221-30.patch, failed testing.

Manjit.Singh’s picture

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 ;)

sxnc’s picture

FileSize
750 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,610 pass(es). View

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.

Wim Leers’s picture

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Needs work
+++ 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?

rudraram’s picture

FileSize
759 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,816 pass(es). View
740 bytes

Indented as suggested by @joelpittet.

rudraram’s picture

Status: Needs work » Needs review
Cottser’s picture

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):

idebr’s picture

Status: Needs review » Needs work

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

Dom.’s picture

Here is a patch with all last comments together. There is no interdiff because every lined has been changed as per the following :

  • - Similar to patch #26 in the markup which fits best with @emma-maria #17.
  • - Similar to patch #36 since if statements has been added to not include markup if the region is empty.
  • - Small change added to CSS to fix comment by @idebr #14 in the case where featured-top is present.




Before patch #40:




After patch #40 with no featured top:




After patch #40 with featured top:

Dom.’s picture

Status: Needs work » Needs review
idebr’s picture

Status: Needs review » Needs work

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

Dom.’s picture

Status: Needs work » Needs review

@idebr : it is a change in hightlighted region background color depending if there are something inside the featured-top region, no matter what is inside the hightlighted region: messages or custom block or whatever.
Isn't it what expected ?

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
61.9 KB
117.59 KB

#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

.

Also @idebr is correct! Bartik has some particular styling for the Highlighted region that needs to be updated and included in this patch. If the Featured Top region exists with it's full width grey background the Highlighted region needs to have that background colour too. Right now the code is referencing the wrong selector so the styles are currently broken, see below...
 

 
The current code that does this....

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

As we are adding markup to the region we need to switch the selector to .has-featured-top .highlighted and also as this is code for the component "highlighted" we need to create a new component file for this and add the code there.

Dom.’s picture

Status: Needs work » Needs review

@emma.maria regarding #44 about background color of hightlighting region: this is the CSS correction I made in patch #40, see last screenshot.

@emma.maria regarding #44 about markup: this is also what does patch #40 (see below) and so does the patch #26 (patch #36 is the wrong one regarding region-... class). Indeed this <div class="region region-.xxx> is properly added by region.html.twig in system/template which is the correct behaviour.
Thus using #40 you will have the correct markup, using patch #36 you will have the div twice.

@emma.maria and @idebr: please could you eventually apply patch #40 and review it ?

emma.maria’s picture

@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.’s picture

Thanks emma, sorry for the missunderstanding !

emma.maria’s picture

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

emma.maria’s picture

Status: Needs review » Active
emma.maria’s picture

Status: Active » Needs review

Trying to fire testbot on the patch in #40.

emma.maria’s picture

FileSize
1.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,037 pass(es). View

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

emma.maria’s picture

Issue summary: View changes
FileSize
118.49 KB
143.91 KB

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'

emma.maria’s picture

Status: Needs review » Reviewed & tested by the community
emma.maria’s picture

Title: Highlighted region has not fixed width layout on Bartik theme » The highlighted region in Bartik is missing layout styles.
emma.maria’s picture

Issue summary: View changes

Sneaking in the missing Beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed f03eba7 on 8.0.x
    Issue #2462221 by emma.maria, Dom., rudraram, jp.stacey, Manjit.Singh,...

Status: Fixed » Closed (fixed)

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