We have in Seven's page.html.twig file:
{% if messages %}
<div id="console" class="clearfix">{{ messages }}</div>
{% endif %}
and in Seven's style.css:
/**
* Console.
*/
#console {
margin: 9px 0 10px;
}Using ID selectors as styling hooks are bad practice according to our CSS coding standards. We should use classes.
Anyway I do not know why we use a wrapper element to add some margin around messages. If there is no other goal of this wrapper than providing the margin than we may remove it and add the margin to messages instead(?)
(See this comment #2017257-50: Create generic layout classes also.)
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | 2321121_after.png | 162.11 KB | csakiistvan |
| #21 | 2321121_before.png | 139.02 KB | csakiistvan |
| #18 | 2321121_refactor_to_class_18.patch | 889 bytes | csakiistvan |
Comments
Comment #1
lewisnymanI think we should remove it. I'm not sure what it's purpose is.
Comment #2
gábor hojtsyWell, the only purpose as is displayed is that it contains the messages. It clears floats and puts a margin around messages in general. If you put margins on messages one by one, then they will be set apart from each other. If you put a margin around the first message appearing on the page and the last, then when multiple message areas are used, such as with #2318381: Message not styled properly on interface translation page , then the margin will not appear properly. So thats why.
Comment #3
lewisnymanWe don't have anything floating within messages anymore, but let's assume we need to keep it it for vertical spacing. The correct class inline with our CSS standards would be
.messages__containerComment #4
lewisnymanComment #5
gábor hojtsyYeah that would make it reusable in #2318381: Message not styled properly on interface translation page .
Comment #6
lewisnymanComment #7
rteijeiro commentedLet's tag it with novice tags.
Comment #8
csakiistvanComment #11
gábor hojtsyIMHO we should update the comment as well. Since we did not know what console was for in the first place, it is not disambiguating to say on a comment that this is the "Console". Probably based on the class names, there is no need for a comment? It looks pretty trivially the message container :)
Comment #12
thamasSwitching to class from ID is a good idea but I still do not understand why we need this wrapper div. Lewis stated that we do not need to clear any floats anymore. So the wrapper only adds margin "around" the top and bottom of message or messages. But to get this result we do no need a wrapper! We can define this margins in messages class.
I do not understand the answer in #2 about "setting apart messages". Vertical margins are collapsed by browsers and there is also a
rule in system.theme.css which sets the distance between messages.
Otherwise the comment should not say that "this is the messages container" but the purpose of this container – and that is the answer to my question(?).
Comment #13
thamasBy the way we have some other id-s in page.html.twig such as #branding, #content, #help, #footer and some styling applied in style.css to #barnding and #help. Maybe we should change them from id to class too. (Have anyone created an issue about them yet?)
Comment #14
gábor hojtsyOk how would you define those margins on the messages?
Comment #15
thamasComment #16
csakiistvan#14 we add this margin to .messages class
Edit: but only in seven theme, i think its better
Comment #17
thamasSimply adding
margin: 9px 0px 10px;to the .messages class (and disable the margin defined by #console) seems working to me.Comment #18
csakiistvanComment #19
lewisnymanGreat! Happy to RTBC once we get some before/after screenshots to ensure it's identical.
Comment #20
lewisnymanComment #21
csakiistvanComment #22
thamasHm. What if we add the margin in system.theme.css instead of style.css of Seven?
Comment #23
lewisnymanHow does Bartik/Stark handle spacing now? I'd usually steer clear of adding more CSS for all themes but as messages are usually fully styled on every theme maybe it makes sense?
Comment #24
lewisnymanThis issue died a little. Let's just stick with Seven for now. No reason to open the scope up here.
Comment #25
thamas@jamesquinton made the CSS change in #2213583-84: Misaligned Icons in Drupal 8.x - it needs review. (And the twig file change should be done too.)
Comment #26
gábor hojtsyDuplicate of #2213583: Misaligned Icons in Drupal 8.x then, no?
Comment #27
csakiistvanyeah