Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#52 | messages-with-featured-top.png | 143.91 KB | emma.maria |
#52 | messages.png | 118.49 KB | emma.maria |
#51 | fixed-size-highlighted-region-2462221-51.patch | 1.19 KB | emma.maria |
#44 | highlighted-with-featured-top.png | 117.59 KB | emma.maria |
#44 | highlighted-markup.png | 61.9 KB | emma.maria |
Comments
Comment #1
Dom. CreditAttribution: Dom. commentedComment #2
Dom. CreditAttribution: Dom. commentedComment #3
joelpittetRTBC from me but assigning to emma for a second look.
Thanks @Dom.
Comment #4
Wim LeersWe 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.
Comment #5
joelpittet@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.
Comment #6
Dom. CreditAttribution: Dom. commented@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.
Comment #7
Wim LeersI'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:i.e. much like
core/modules/text/templates/field--text.html.twig
.Comment #8
joelpittetNo 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
Comment #9
Manjit.Singhsubscribing this issue...
Comment #10
Wim Leers#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 ;)
Comment #11
davidhernandezMy 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.
Comment #12
idebr CreditAttribution: idebr at One Shoe commentedI 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.
Comment #13
Wim LeersAha! 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 :)
Comment #14
idebr CreditAttribution: idebr at One Shoe commented@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:
Comment #15
Wim LeersAlright, 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.
Comment #16
emma.mariaI 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.
Comment #17
emma.mariaI 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...
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.
Comment #18
emma.mariaComment #19
emma.mariaComment #20
Dom. CreditAttribution: Dom. commented@emma.maria regarding #17: Should we renaim the region to "Informations" or something ? (even remove it and assign Message block to Feature Top ?)
Comment #21
LewisNyman@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.
Comment #22
Dom. CreditAttribution: Dom. commented@LewisNyman is it that bad ?
Comment #23
LewisNyman@Dom. Yeah it looks bad, but mostly we shouldn't be changing the design in these issues :\
Comment #24
emma.mariaI have created this issue to rename the Messages region...
#2470807: Rename the default "Messages" region for all themes to "Highlighted"
Comment #25
emma.mariaPostponing 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".
Comment #26
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThis 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 :)
Comment #27
emma.mariaComment #28
emma.mariaNow 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.
Comment #29
Manjit.SinghComment #30
Manjit.Singhrerolling a #26 patch and removing message region from
bartik.info.yml
file.Comment #32
Manjit.Singhohh 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 ;)
Comment #33
sxnc CreditAttribution: sxnc as a volunteer commentedI 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.
Comment #34
Wim LeersComment #35
joelpittetJust needs to be indented once more inside the if statement.
Emma does this approach suit the needs for this issue?
Comment #36
rudraram CreditAttribution: rudraram at Axelerant commentedIndented as suggested by @joelpittet.
Comment #37
rudraram CreditAttribution: rudraram at Axelerant commentedComment #38
star-szrI 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):
Comment #39
idebr CreditAttribution: idebr at One Shoe commentedBoth #26 and #36 suffer from an implementation issue I explained in #14
Comment #40
Dom. CreditAttribution: Dom. commentedHere is a patch with all last comments together. There is no interdiff because every lined has been changed as per the following :
Before patch #40:
After patch #40 with no featured top:
After patch #40 with featured top:
Comment #41
Dom. CreditAttribution: Dom. commentedComment #42
idebr CreditAttribution: idebr at One Shoe commentedThe solution in #40 results in a change in background-color for the highlighted region depending if there are messages on the page.
Comment #43
Dom. CreditAttribution: Dom. commented@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 ?
Comment #44
emma.maria#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....
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.Comment #45
Dom. CreditAttribution: Dom. commented@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 byregion.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 ?
Comment #46
emma.maria@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.
Comment #47
Dom. CreditAttribution: Dom. commentedThanks emma, sorry for the missunderstanding !
Comment #48
emma.maria@Dom No worries, thanks for the patch! :)
Comment #49
emma.mariaComment #50
emma.mariaTrying to fire testbot on the patch in #40.
Comment #51
emma.mariaRe-uploading the patch from #40 with no changes. I cannot for the life of me get testbot to acknowledge it.
Comment #52
emma.mariaThe 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'
Comment #53
emma.mariaComment #54
emma.mariaComment #55
emma.mariaSneaking in the missing Beta evaluation.
Comment #56
alexpottThis 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!