Problem/Motivation

Bartik's code needs to meet current Drupal coding standards.

Proposed resolution

This issue takes a specific section of Bartik's code, it acts as a "component" issue and improves that section of code with minimal impact on the rest of Bartik's codebase.

This issue aims to clean up and properly format the CSS and templates files without breaking Bartik visually.

This issue primarily looks at the css/components/messages.css file.

Work that needs to be included in patches for this issue are fully outlined in the META issue #1342054: [META] Clean up templates and CSS.

Remaining tasks

  • Write a patch
  • Review the patch - code review and visual changes
  • Upload screenshots to show nothing/something is broken on the frontend

User interface changes

None, we are cleaning up CSS and markup in templates. The use of Bartik's UI and design will stay the same.

API changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is a code clean up overhaul of a theme.
Unfrozen changes Unfrozen because it only refactors CSS and templates, no changes to UI or APIs
Prioritized changes The main goal of this issue is usability and performance. We want Bartik code to be up to date and something to be proud of.
Disruption No disruption it's only refactoring code not changing how to use the theme

Test Instructions

This is the component that needs to be visually tested with and without the patch applied. No CSS testing required.

CommentFileSizeAuthor
#59 messages-2456805-59.patch3.46 KBrachel_norfolk
#59 interdiff-2456805-49-59.txt458 bytesrachel_norfolk
#57 message-after-1.png24.04 KBManjit.Singh
#57 message-before-1.png24 KBManjit.Singh
#57 message-after.png29.33 KBManjit.Singh
#57 message-before.png32.97 KBManjit.Singh
#56 AfterScreenshot.png167.77 KBmgifford
#55 interdiff-2456805-49-55.txt458 bytesrachel_norfolk
#55 messages-2456805-55.patch26.34 KBrachel_norfolk
#54 screenshot.png125.83 KBdcor
#53 welcome_msg.png13 KBdcor
#49 interdiff-2456805-43-49.txt861 bytesrachel_norfolk
#49 messages-2456805-49.patch3.44 KBrachel_norfolk
#48 interdiff-2456805-39-45.txt1.4 KBrachel_norfolk
#45 messages-2456805-43.patch3.48 KBVidushi Mehta
#39 messages-2456805-39.patch3.53 KBrachel_norfolk
#39 interdiff-2456805-36-39.txt477 bytesrachel_norfolk
#36 interdiff.txt426 bytessaki007ster
#36 messages-2456805-36.patch3.43 KBsaki007ster
#34 Screen_Shot_2015-09-14_at_12_37_53.png261.27 KBemma.maria
#34 Screen_Shot_2015-09-14_at_12_37_35.png237.03 KBemma.maria
#33 no-msg-home-with-patch.png183.37 KBemma.maria
#33 no-msg-home-without-patch.png202.25 KBemma.maria
#31 messages-2456805-31.patch3.45 KBManjit.Singh
#30 messages-2456805-30.patch2.91 KBManjit.Singh
#26 2456805-18-26.txt427 bytesrudraram
#26 messages-2456805-26.patch3.45 KBrudraram
#21 interdiff-18-21.txt427 bytesChandeepKhosa
#21 clean_up_the_messages-2456805-21.patch1.06 MBChandeepKhosa
#18 2456805-15-18.txt2.26 KBrudraram
#18 messages-2456805-18.patch3.47 KBrudraram
#15 messages-2456805-15.patch2.57 KBrudraram
#5 messages-2456805-4.patch2.57 KBemma.maria
#1 2456805-1.patch2.53 KBidebr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr’s picture

Status: Active » Needs review
FileSize
2.53 KB

The patch also includes a bugfix for messages that are currently displayed over the full width of the page that I missed in #2398451: Clean up "layout" CSS in Bartik (sorry).

idebr’s picture

emma.maria’s picture

Issue tags: +Needs reroll

Patch does not apply - needs reroll.

emma.maria’s picture

Issue summary: View changes
emma.maria’s picture

Title: Clean up the "messages" component in Bartik » Clean up and fix bugs within the "messages" component in Bartik
Issue tags: -Needs reroll
FileSize
2.57 KB

I have rerolled the patch and cleaned up the issue summary.

emma.maria’s picture

Status: Needs review » Needs work

The problem with the patch is that we can't apply layout-container to just the messages block. If the user adds anything else to the Messages region they do not get the contained layout. We need to tackle this on the region level.

idebr’s picture

Before #2398451: Clean up "layout" CSS in Bartik was committed, the container css used to apply to #messages div.section. This patch should probably mimic this behavior. Disclaimer: I have not checked if this is currently the case as I do not have a development environment at hand.

emma.maria’s picture

Yes I would like the region to be wrapped in the layout markup and be given the styles at that level. See https://www.drupal.org/node/2462221#comment-9816901.

We do not the messages ID in status.messages.html.twig. We probably do not need to extend the messages template from Classy at all now.

The horrible problem we have now is that we have two messages components in Bartik. The region and the block.

I would love to change the Messages region name as a region name shouldn't represent the default content within it.

emma.maria’s picture

Issue tags: +drupaldevdays
jp.stacey’s picture

Looking at #2462221: The highlighted region in Bartik is missing layout styles., the change requested to page.html.twig should just be:

--- a/core/themes/bartik/templates/page.html.twig
+++ b/core/themes/bartik/templates/page.html.twig
@@ -109,7 +109,13 @@
         {{ page.primary_menu }}
       </div>
     </header>
-    {{ page.messages }}
+    {% if page.messages %}
+      <div class="region region-messages">
+        <aside class="layout-container section clearfix" role="complementary">
+          {% page.messages %}
+        </aside>
+      </div>
+    {% endif %}
     {% if page.featured_top %}
       <div class="featured-top">
         <aside class="featured-top__inner section layout-container clearfix" role="complementary">

However, clicking through from #2462221: The highlighted region in Bartik is missing layout styles. to #2470807: Rename the default "Messages" region for all themes to "Highlighted", it looks like the latter is blocking the former, which is then blocking this. Is that correct?

If so, it's a bit confusing: can we roll 2462221 into this and mark it as blocked? Although 2462221 is technically a change to behaviour, it's really a bugfix, as that behaviour was presumably not the case in D7!

For clarity (for anyone else landing here!) it's certainly worth marking this ticket as ultimately blocked by 2470807, so I've added that as a related ticket now. Any other way we can show this is blocked?

jp.stacey’s picture

Oh, two more things:

1. I can confirm the patch in #5 still applies fine
2. Once those other tickets are unblocked, is there any more work needed to be done here? Or any other changes we can move ahead with in the mean time?

emma.maria’s picture

Status: Needs work » Postponed

Postponing this until the #2462221: The highlighted region in Bartik is missing layout styles. has been committed.

emma.maria’s picture

Title: Clean up and fix bugs within the "messages" component in Bartik » Clean up the "messages" component in Bartik
mgifford’s picture

Status: Postponed » Needs work
rudraram’s picture

Status: Needs work » Needs review
FileSize
2.57 KB

Patch #5 getting rejected for some strange reason. Re-rolling it.

emma.maria’s picture

Status: Needs review » Needs work
Issue tags: +Novice

This issue needs work as the Messages region is now called Highlighted – #2462221: The highlighted region in Bartik is missing layout styles..

There are a few code issues that need cleaning up.

  1. +++ b/core/themes/bartik/css/components/messages.css
    @@ -1,18 +1,20 @@
    +.has-featured-top .region-messages {
       background: #f0f0f0;
       background: rgba(30, 50, 10, 0.08);
     }
    

    .region-messages is now .region-highlighted. Can this code be amended plus moved to a new CSS file called <strong>highlighted.css</strong>.

  2.  

  3. +++ b/core/themes/bartik/css/components/messages.css
    @@ -1,18 +1,20 @@
    +.messages-wrapper {
    +  padding: 20px 0 5px;
    

    The wrapper is an element of the messages component. This class needs to be changed in the CSS and template to .messages__wrapper to follow BEM standards.

  4.  

  5. +++ b/core/themes/bartik/css/components/messages.css
    @@ -1,18 +1,20 @@
    +.messages {
       margin: 8px 15px 8px 23px; /* LTR */
     }
    

    This margin is for the region layout and not for messages. Can this please be moved to the .region-highlighted selector within the highlighted.css file.

killua99’s picture

I'll take a look late today.

rudraram’s picture

@emma.maria I have updated the patch as per your suggestion. Attaching interdiff.

rudraram’s picture

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

Status: Needs review » Needs work

Thanks @rudraram the changes are great!

One small improvement and this is RTBC.

+++ b/core/themes/bartik/css/components/highlighted.css
@@ -0,0 +1,16 @@
+[dir="rtl"] .region-highlighted {
+  margin-right: 23px;
+  margin-left: 15px;
+}

Can this be declared as margin: .... with the right order for RTL, to match the LTR declaration please.

ChandeepKhosa’s picture

Status: Needs work » Needs review
FileSize
1.06 MB
427 bytes

Here is the new patch I created. The margin has been written in the normal format (Top, Right, Bottom, Left), but reversed as it is RTL not LTF. I hope this is correct.

Status: Needs review » Needs work

The last submitted patch, 21: clean_up_the_messages-2456805-21.patch, failed testing.

emma.maria’s picture

@ChandeepKhosa you have unfortunately added things to the patch that should be within a .gitignore file, everything within sites/default... etc that should be ignored by git.
Apologies if my help earlier contributed to this. Next time check what files are untracked in git with git status and only add the things that are relevant to the patch :-)

Check the "Create a ~/.gitignore file for 8.x" section on this d.o page https://www.drupal.org/documentation/git/configure.

emma.maria’s picture

So we need a new patch based on #18 containing the small improvement explained in #20. This issue is so close!

Thanks.

ChandeepKhosa’s picture

Assigned: Unassigned » ChandeepKhosa

Thanks, I'm taking care of this now

rudraram’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
427 bytes

@emma.maria Uploading the patch as per the suggestions.

ChandeepKhosa’s picture

Assigned: ChandeepKhosa » Unassigned

..

Manjit.Singh’s picture

Issue tags: -Novice

changes added in #26 patch as per suggestion in #20 by emma. Thanks @Rudraram !!
waiting for testbot ;) then will move it to RTBC.

emma.maria’s picture

Status: Needs review » Needs work
+++ b/core/themes/bartik/css/components/highlighted.css
@@ -0,0 +1,15 @@
+.region-highlighted {
+  margin: 8px 15px 8px 23px; /* LTR */
+}
+[dir="rtl"] .region-highlighted {
+  margin: 0 23px 0 15px;
+}

Wait don't RTBC yet! The RTL margin is incorrect. It needs to keep the 8px values to match the LTR and not have 0 values.

@rudraram Here is the RTL styles documentation if you need any help. You were so so close with this! Let's get this fixed :-)

Manjit.Singh’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

changes added @emma. Please verify.

Manjit.Singh’s picture

FileSize
3.45 KB

oops Last patch was missed some code :( ... here is the updated version.

rudraram’s picture

@emma.maria Thanks for the documentation. It will help in the future.

Thanks for the re-roll @manjit.singh

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Novice
FileSize
202.25 KB
183.37 KB

Bah I've just noticed that even when region-highlighted has nothing in it, it prints. This has already been raised as a separate issue!
However this means that we can't add vertical margin to regions.

See below:
 

 

 
But noticing that has also uncovered that we are adding margin only specific to the layout of messages (the 23px on the left) to region-highlighted.

.region-highlighted {
  margin: 8px 15px 8px 23px; /* LTR */
}

Therefore to rectify this let's keep the region Highlighted layout margin as the standard that most regions have:
margin : 0 15px;

And then add the rest of the pixels leftover from margin: 8px 15px 8px 23px currently on .region-highlighted to the padding declaration of .messages__wrapper.

Then we will be finished with this issue :)

emma.maria’s picture

saki007ster’s picture

Assigned: Unassigned » saki007ster
saki007ster’s picture

I have updated the patch as per the comments from @emma.maria in #33 .

saki007ster’s picture

Assigned: saki007ster » Unassigned
Status: Needs work » Needs review
rachel_norfolk’s picture

Assigned: Unassigned » rachel_norfolk

Working on this at Drupalcon Barcelona...

rachel_norfolk’s picture

Assigned: rachel_norfolk » Unassigned
FileSize
477 bytes
3.53 KB

Tiny tidy up of messages.css to ensure the now missing 8 pixels are re-added into the padding of .messages__wrapper.

Note that mikebell_ also needs crediting with this...

mikebell_’s picture

Tagging myself in.

idebr’s picture

Status: Needs review » Needs work

Thanks for working on this issue! I found a few things that should be updated:

  1. +++ b/core/themes/bartik/css/components/highlighted.css
    @@ -0,0 +1,15 @@
    +.region-highlighted {
    +  margin : 0 15px; /* LTR */
    +}
    +[dir="rtl"] .region-highlighted {
    +  margin : 0 15px;
    +}
    

    Since the RTL styling is the same as the LTR styling, it can be removed. Nitpick: the colon is placed directly after the attribute according to the Drupal coding standards.

  2. +++ b/core/themes/bartik/css/components/messages.css
    @@ -1,18 +1,13 @@
    +.messages__wrapper {
    +  padding: 20px 0 5px 8px;
    ...
    -div.messages {
    -  margin: 8px 15px 8px 23px; /* LTR */
    -}
    

    The current version has a larger margin around the messages. Although I don't disagree with this change, according to the issue summary no changes should be made to the current design.

  3. +++ b/core/themes/bartik/css/components/messages.css
    @@ -1,18 +1,13 @@
    +
    +[dir="rtl"] .messages__wrapper {
    +  padding: 20px 8px 5px 0;
    
    +++ b/core/themes/bartik/css/maintenance-page.css
    @@ -54,11 +53,11 @@ body.maintenance-page {
    +.maintenance-page .messages-wrapper {
    

    Nitpick: [dir="rtl"] styling is placed directly after the previous selector without an additional line break.

  4. +++ b/core/themes/bartik/css/maintenance-page.css
    @@ -54,11 +53,11 @@ body.maintenance-page {
    +.maintenance-page .messages-wrapper {
    

    This selector does not exist, although messages-wrapper is technically correct. messages__wrapper implies it is a child element of messages according to the BEM naming scheme.

  5. +++ b/core/themes/bartik/css/maintenance-page.css
    @@ -54,11 +53,11 @@ body.maintenance-page {
    +.maintenance-page .messages {
       margin: 0;
     }
    

    .messages has no margin by default on the maintenance page, so this selector can be removed.

Manjit.Singh’s picture

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

#2 2. Messages have always had a larger space on the left to account for the vertical strip of colour before them. We are moving this to the messages wrapper as it was set on the region that any block can use. Please leave the extra space for correct horizontal alignment of messages.

rachel_norfolk’s picture

Hi Manjit.Singh - are you working on this issue today? I wanted to spend some time on it this afternoon if not?

Are you here in Barcelona? Would be good to meet in person as we met a few times in the queue...

Vidushi Mehta’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
Issue tags: -Novice, -Needs manual testing, -Needs screenshots
FileSize
3.48 KB

All changes addresses in #41 are added in patch. Please review.

Manjit.Singh’s picture

@rachel_norfolk Unfortunately, i am not in Barcelona :) but yeah you can start work on it.

emma.maria’s picture

@Vidushi Mehta Please do not remove tags that still need to be applied to the issue.

Also in future can you please add an interdiff file along with the patch so we can easily check the changes that you made — Creating an interdiff

Thanks!

rachel_norfolk’s picture

FileSize
1.4 KB

Interdiff for history of last two patches. A couple of things outstanding which I'm now implementing...

rachel_norfolk’s picture

The last submitted patch, 45: messages-2456805-43.patch, failed testing.

dcor’s picture

Assigned: Unassigned » dcor

I am manually testing this issue.

dcor’s picture

Issue summary: View changes
FileSize
13 KB

I am manually testing this issue. (I have @emma.maria sitting next to me.)

dcor’s picture

Assigned: dcor » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Needs screenshots
FileSize
125.83 KB

Unfortunately, the before and after patch of the DSM are not the same - spacing issue above the DSM.

rachel_norfolk’s picture

Status: Needs work » Needs review
FileSize
26.34 KB
458 bytes

Thanks dcor - you spotted my missing margin!

mgifford’s picture

FileSize
167.77 KB

Here's a screenshot with the latest patch. This looks good to me.

Manjit.Singh’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
32.97 KB
29.33 KB
24 KB
24.04 KB

good to go.. here are some screenshots of manual testing.

dasd

dasd

dasd

dasd

idebr’s picture

Status: Reviewed & tested by the community » Needs work

The patch in #55 also contains unrelated changes, probably from another issue.

rachel_norfolk’s picture

Status: Needs work » Needs review
FileSize
458 bytes
3.46 KB

Ah yes - I accidentally (?) managed to somehow apply graphql into the patch!

Repaired.

pjbaert’s picture

Status: Needs review » Reviewed & tested by the community

All unrelated changes are gone in this latest patch. This can go back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ac24ea6 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

  • alexpott committed ac24ea6 on 8.0.x
    Issue #2456805 by rachel_norfolk, rudraram, Manjit.Singh, emma.maria,...

Status: Fixed » Closed (fixed)

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

Vidushi Mehta’s picture