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
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.
Comment | File | Size | Author |
---|---|---|---|
#59 | messages-2456805-59.patch | 3.46 KB | rachel_norfolk |
#59 | interdiff-2456805-49-59.txt | 458 bytes | rachel_norfolk |
#57 | message-after-1.png | 24.04 KB | Manjit.Singh |
#57 | message-before-1.png | 24 KB | Manjit.Singh |
#57 | message-after.png | 29.33 KB | Manjit.Singh |
Comments
Comment #1
idebr CreditAttribution: idebr commentedThe 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).
Comment #2
idebr CreditAttribution: idebr commentedComment #3
emma.mariaPatch does not apply - needs reroll.
Comment #4
emma.mariaComment #5
emma.mariaI have rerolled the patch and cleaned up the issue summary.
Comment #6
emma.mariaThe 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.
Comment #7
idebr CreditAttribution: idebr commentedBefore #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.Comment #8
emma.mariaYes 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.
Comment #9
emma.mariaComment #10
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedLooking at #2462221: The highlighted region in Bartik is missing layout styles., the change requested to page.html.twig should just be:
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?
Comment #11
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedOh, 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?
Comment #12
emma.mariaPostponing this until the #2462221: The highlighted region in Bartik is missing layout styles. has been committed.
Comment #13
emma.mariaComment #14
mgiffordComment #15
rudraram CreditAttribution: rudraram at Axelerant commentedPatch #5 getting rejected for some strange reason. Re-rolling it.
Comment #16
emma.mariaThis 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.
.region-messages is now .region-highlighted. Can this code be amended plus moved to a new CSS file called
<strong>highlighted.css</strong>
.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.This margin is for the region layout and not for messages. Can this please be moved to the
.region-highlighted
selector within thehighlighted.css
file.Comment #17
killua99 CreditAttribution: killua99 commentedI'll take a look late today.
Comment #18
rudraram CreditAttribution: rudraram at Axelerant commented@emma.maria I have updated the patch as per your suggestion. Attaching interdiff.
Comment #19
rudraram CreditAttribution: rudraram at Axelerant commentedComment #20
emma.mariaThanks @rudraram the changes are great!
One small improvement and this is RTBC.
Can this be declared as
margin: ....
with the right order for RTL, to match the LTR declaration please.Comment #21
ChandeepKhosa CreditAttribution: ChandeepKhosa at 2Toucans commentedHere 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.
Comment #23
emma.maria@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.
Comment #24
emma.mariaSo we need a new patch based on #18 containing the small improvement explained in #20. This issue is so close!
Thanks.
Comment #25
ChandeepKhosa CreditAttribution: ChandeepKhosa at 2Toucans commentedThanks, I'm taking care of this now
Comment #26
rudraram CreditAttribution: rudraram at Axelerant commented@emma.maria Uploading the patch as per the suggestions.
Comment #27
ChandeepKhosa CreditAttribution: ChandeepKhosa at 2Toucans commented..
Comment #28
Manjit.Singhchanges added in #26 patch as per suggestion in #20 by emma. Thanks @Rudraram !!
waiting for testbot ;) then will move it to RTBC.
Comment #29
emma.mariaWait 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 :-)
Comment #30
Manjit.Singhchanges added @emma. Please verify.
Comment #31
Manjit.Singhoops Last patch was missed some code :( ... here is the updated version.
Comment #32
rudraram CreditAttribution: rudraram at Axelerant commented@emma.maria Thanks for the documentation. It will help in the future.
Thanks for the re-roll @manjit.singh
Comment #33
emma.mariaBah 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.
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 :)
Comment #34
emma.mariaComment #35
saki007sterComment #36
saki007sterI have updated the patch as per the comments from @emma.maria in #33 .
Comment #37
saki007sterComment #38
rachel_norfolkWorking on this at Drupalcon Barcelona...
Comment #39
rachel_norfolkTiny 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...
Comment #40
mikebell_ CreditAttribution: mikebell_ as a volunteer commentedTagging myself in.
Comment #41
idebr CreditAttribution: idebr commentedThanks for working on this issue! I found a few things that should be updated:
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.
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.
Nitpick: [dir="rtl"] styling is placed directly after the previous selector without an additional line break.
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.
.messages has no margin by default on the maintenance page, so this selector can be removed.
Comment #42
Manjit.SinghComment #43
emma.maria#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.
Comment #44
rachel_norfolkHi 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...
Comment #45
Vidushi Mehta CreditAttribution: Vidushi Mehta commentedAll changes addresses in #41 are added in patch. Please review.
Comment #46
Manjit.Singh@rachel_norfolk Unfortunately, i am not in Barcelona :) but yeah you can start work on it.
Comment #47
emma.maria@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!
Comment #48
rachel_norfolkInterdiff for history of last two patches. A couple of things outstanding which I'm now implementing...
Comment #49
rachel_norfolkOkay, a couple of cleanup operations and re-applied the comment #43 as requested by emma-maria.
Comment #52
dcor CreditAttribution: dcor commentedI am manually testing this issue.
Comment #53
dcor CreditAttribution: dcor commentedI am manually testing this issue. (I have @emma.maria sitting next to me.)
Comment #54
dcor CreditAttribution: dcor commentedUnfortunately, the before and after patch of the DSM are not the same - spacing issue above the DSM.
Comment #55
rachel_norfolkThanks dcor - you spotted my missing margin!
Comment #56
mgiffordHere's a screenshot with the latest patch. This looks good to me.
Comment #57
Manjit.Singhgood to go.. here are some screenshots of manual testing.
Comment #58
idebr CreditAttribution: idebr commentedThe patch in #55 also contains unrelated changes, probably from another issue.
Comment #59
rachel_norfolkAh yes - I accidentally (?) managed to somehow apply graphql into the patch!
Repaired.
Comment #60
pjbaertAll unrelated changes are gone in this latest patch. This can go back to RTBC.
Comment #61
alexpottCommitted ac24ea6 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation.
Comment #64
Vidushi Mehta CreditAttribution: Vidushi Mehta as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented