Problem/Motivation
Our current CSS is a mish-mash of styles that don't follow the SMACSS categorization standards that are now part of our CSS standards. See http://drupal.org/node/1887918
Proposed resolution
We need to reoganize our styles so that they are loaded in this order:
- "Base" HTML element styling
- "Layout" styling
- Component styling and their associated state and skin styles
- Un-associated state styles
- Un-associated skin styles
This is the second step in a proposed solution outlined at #1921610: [Meta] Architect our CSS. In short, we are moving all of core's "base" HTML element styling into a library so that we can state "Modules must never have base HTML element styling".
Remaining tasks
This patch is blocked by #1924368: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants.
After this issue's completion, we still have all the other steps outlined at #1921610: [Meta] Architect our CSS.
User interface changes
none
API changes
Adds a new drupal.base.css library that is loaded by default.
Comment | File | Size | Author |
---|---|---|---|
#75 | 1924430-75-base-html.patch | 5.89 KB | echoz |
#71 | 1924430-71-base-html.patch | 5.89 KB | echoz |
#63 | 1924430-63-base-html.patch | 5.88 KB | JohnAlbin |
#60 | 1924430-60-base-html.patch | 6.93 KB | JohnAlbin |
#57 | drupal-base-css-1924430-57.patch | 6.9 KB | echoz |
Comments
Comment #1
JohnAlbinHere's the patch.
Comment #3
JohnAlbinFound some more base styling in the Overlay module. *sigh* So this is now in drupal.base-rtl.css:
Note, I have no idea why this new ruleset is needed BECAUSE THERE ARE NO COMMENTS FOR THIS RULESET. It was added in this patch with no discussion or comment: #766170-5: Overlay lacks rtl styling
Comment #5
sunCan we find a better file prefix than "drupal." ?
How about html.base.css?
Comment #6
Shyamala CreditAttribution: Shyamala commentedAdding tags
Comment #7
ry5n CreditAttribution: ry5n commentedSince this base CSS is now a core library, a Drupal `namespace` makes sense, doesn't it?
Comment #8
kim.pepperI re-rolled this against head. There were a couple of conflicts in there that I'm not too sure about, so this will definitely need some review.
Kim
Comment #10
kim.pepper#8: 1924430-8-drupal.base_.css_.patch queued for re-testing.
Comment #12
kim.pepper#8: 1924430-8-drupal.base_.css_.patch queued for re-testing.
Comment #13
kim.pepperCSS_BASE is not being defined anywhere. Just re-read the summary and can see it is part of #1924368: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants
So much for a quick re-roll!
Comment #14
JohnAlbinNow that #1924368: Rename old CSS constants to CSS_AGGREGATE_* and add new CSS weight constants is committed, this patch needs testing.
Comment #15
JohnAlbin> Since this base CSS is now a core library, a Drupal `namespace` makes sense, doesn't it?
It makes sense to me. And Morten was the one who actually suggested this name originally. It is how Drupal core thinks base HTML styling should be done.
Comment #16
kim.pepper#8: 1924430-8-drupal.base_.css_.patch queued for re-testing.
Comment #18
kim.pepperRe-roll.
Comment #19
JohnAlbinThe patch in #18 is missing the
main
element that was added to the block of HTML5 elements and the newinput
rule that was added to system.base.css.Here's an updated patch.
Comment #20
rteijeiro CreditAttribution: rteijeiro commentedThe #19 patch applies well and it seems nothing is broken in the css styling.
Also the drupal.base.css is loaded.
Is there something more I should take into account?
Comment #21
rteijeiro CreditAttribution: rteijeiro commentedFix status.
Comment #22
ry5n CreditAttribution: ry5n commentedThe CSS is all retained, it’s a straight consolidation, so that looks good.
I looked through system.module and found no straggling base styles except the new
<details>
and<summary>
elements in system.theme.css. We can probably leave that for now, since<details>
needs a js polyfill, has internal elements styled with classes, and may be better styled as a component anyway.I can’t comment on the changes to theme.maintenance.inc or system.module, although everything seems to look correct with the patch applied.
Comment #23
izus CreditAttribution: izus commented#19: 1924430-19-drupal.base_.css_.patch queued for re-testing.
Comment #25
izus CreditAttribution: izus commentedHi,
rerolling #19
Thanks
Comment #26
rteijeiro CreditAttribution: rteijeiro commentedThe patch now applies well.
Comment #27
rteijeiro CreditAttribution: rteijeiro commentedIt seems right for me. Could anyone else confirm that?
Comment #28
echoz CreditAttribution: echoz commentedIf the two patches are compared, we see that #24 is not removing a whole section of system.theme.css that #19 is removing, which is still currently in the file (plus the button element is now present). Assuming #19 was correct at that time, this needs work.
Comment #29
izus CreditAttribution: izus commentedOh i missed that, Thanks
Here it is !
Comment #30
echoz CreditAttribution: echoz commentedYou left out drupal.base.css, what this issue is about. Don't forget there is the small ruleset for button, that was added since John's patch that has to go somewhere.
Comment #31
izus CreditAttribution: izus commentedHi,
taking into account the comments in #30.
Thanks
Comment #32
echoz CreditAttribution: echoz commented@izus, it's convenient to set our text editors to end files with a line break, as this is in our coding standards. #31 seems to have missed this, also mysteriously changed a comment, and failed to add that new button ruleset.
This patch should be an accurate rerole of the #19 patch.
I kept the new button ruleset with the button styles that it came with from #1719640: Use 'button' element instead of empty links in system.theme.css since I didn't think we want button {font-size: 1em;} in our drupal.base.css
Comment #34
echoz CreditAttribution: echoz commented#32: drupal-base-css-1924430-32.patch queued for re-testing.
Comment #35
izus CreditAttribution: izus commentedHi,
I reviewed #32 and it seems good for me.
Thanks
Comment #36
alexpottMost of these declarations are in normalize.css and therefore I think might be unnecessary.
From normalize.css...
... so we only need main and menu in the bit that ensures display values for browsers that don't recognize the new elements.
Comment #37
echoz CreditAttribution: echoz commentedGood catch, although currently only Seven uses core's normalize.css. #1839318: Replace drupal.base.css library with normalize.css for all themes is not committed yet but seems like it will be, although that issue has been stalled since mid March.
#32 could go in with a followup to remove the redundant parts when #1839318 gets committed (Seven currently uses both normalize.css and system.base.css with the overlap), or this issue can get postponed until then.
Notes on the html5 elements:
The version of normalize.css in core is out of date.
"main" is now in both v1 and v2 of normalize.
menu is no longer in normalize, we can get rid of it.
https://github.com/necolas/normalize.css/issues/24
Related, hgroup is being removed from html5, that could go too.
Comment #38
echoz CreditAttribution: echoz commentedI opened #1986616: Update normalize.css library with a patch to update core's normalize.css library.
Comment #39
echoz CreditAttribution: echoz commentedIf we want to go forward with committing this then following up with removing redundancies if and when #1839318: Replace drupal.base.css library with normalize.css for all themes gets committed, here is the patch with menu and hgroup removed.
Reference for hgroup removal, http://html5doctor.com/the-hgroup-element/ although this isn't gone from normalize.css.
Comment #40
LewisNymanHere's a re-roll of the patch. This looks pretty good to me? RTBC?
Comment #41
echoz CreditAttribution: echoz commented#40 leaves out drupal.base.css + drupal.base-rtl.css
Comment #42
echoz CreditAttribution: echoz commentedRe-roll of #39
Comment #43
echoz CreditAttribution: echoz commentedRe-roll, system.module has been getting touched often lately.
Comment #45
echoz CreditAttribution: echoz commentedYet another re-roll.
Comment #46
rteijeiro CreditAttribution: rteijeiro commentedChecked. Patch applies well and drupal.base.css is loaded correctly. Go fo RTBC!
Comment #47
echoz CreditAttribution: echoz commentedsystem.module touched again. Testing that it applies should be all that's needed to rtbc again. Let's get this committed!
Comment #48
echoz CreditAttribution: echoz commentedI mistakenly re-rolled before learning that's not needed when just line #s changed. It's the same patch as #45.
Comment #49
LewisNymanThis applies cleanly.
Comment #50
echoz CreditAttribution: echoz commentedneeds re-roll since #1987066: Rename files to match CSS file naming convention
Comment #51
echoz CreditAttribution: echoz commentedre-rolled!
Comment #52
rteijeiro CreditAttribution: rteijeiro commentedPatch applies well and drupal.base.css file loads correctly.
Also tried with a RTL language and drupal.base-rtl.css loads correctly too.
Go for RTBC!!
Comment #53
JohnAlbinMinor bugs discovered. In the latest patch, the button styling seems to have been inadvertently left in system.theme.css. Also, there's some HTML5 elements that are missing from the display: block rule.
Comment #55
echoz CreditAttribution: echoz commented@JohnAlbin,
#37 explains the omission of menu and hgroup. Here's a resource link for hgroup too. http://html5doctor.com/the-hgroup-element/
#32 explains moving the button ruleset, but if you want that in drupal.base.css that's fine with me :-)
Comment #56
JohnAlbinSorry! Didn't realize you were trying to update the ruleset at the same time as moving them. Given #37, go ahead and roll a new patch with menu and hgroup removed from the base stylesheet.
However, the
button
ruleset is a base style and our new CSS guidelines for D8 say you CANNOT put a base style anywhere except in our base stylesheet. So either the ruleset goes in there as-is or the ruleset's selector needs to be rewritten to not be a HTML element. Looking at #1719640: Use 'button' element instead of empty links I see nod_ added it to make the button.link styling. IMO, thefont-size: 1em;
declaration should be moved to system.theme.css’button.link
ruleset and thebutton
ruleset can just be removed.echoz, can you roll a new patch? I can review. :-)
Comment #57
echoz CreditAttribution: echoz commentedHere ya go...
Comment #58
JohnAlbinPerfect! Thanks, echoz!
Comment #59
alexpottNeeds reroll
Comment #60
JohnAlbinRe-rolled.
Comment #61
echoz CreditAttribution: echoz commentedThis was a comment change, and now applies cleanly.
Comment #62
JohnAlbin#2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute landed (yay!), so this needs a re-roll again. :-)
Comment #63
JohnAlbinThis patch removes this now-obviously-redundant ruleset:
Here's documentation to back up my assertion. https://developer.mozilla.org/en-US/docs/Web/CSS/direction says:
And, of course, the patch has been re-rolled so it no longer has a -rtl stylesheet. :-)
Comment #65
pakmanlhIt applies perfect ;)
* Ops, I don't see the last update, sorry
Comment #66
pakmanlhComment #67
pakmanlhComment #68
pakmanlh#63: 1924430-63-base-html.patch queued for re-testing.
Comment #69
pakmanlhFinally, pass the test ;)
Comment #70
alexpottNeeds a reroll
This should be
[dir="rtl"]
see #2030925: quote rtl attribute selectorComment #71
echoz CreditAttribution: echoz commentedreroll
Comment #72
izus CreditAttribution: izus commentedthe patch in #71 seems good !
Thanks
Comment #73
JohnAlbinThis is RTBC from me as well.
Comment #74
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #75
echoz CreditAttribution: echoz commentedneeded re-roll to quote rtl attribute selector since #2030925: quote rtl attribute selector landed.
Comment #76
JohnAlbinThis new patch is RTBC from once the bot says its green.
Comment #77
echoz CreditAttribution: echoz commentedComment #78
alexpottCommitted be502b4 and pushed to 8.x. Thanks!