Problem/Motivation
The BAT (base-admin-theme) file organization we started to do in Drupal 8 was a fantastic idea. See http://drupal.org/node/1089868
It works really well, but its names conflict with the SMACSS categorization we're using in Drupal 8. "base" means something else in SMACSS. So we just need to rename it.
Proposed resolution
Rename Drupal module’s CSS files to match new file naming convention
eg. MODULE.base.css becomes MODULE.module.css.
In addition, since our template files are now in a templates
sub-directory of a module, we should do the same for the CSS. Note that the toolbar, tour and views modules already use a css sub-directory.
Here's the full list of files that need to be moved/renamed:
Old name/location | New name/location |
---|---|
misc/dropbutton/dropbutton.base-rtl.css | misc/dropbutton/dropbutton-rtl.css |
misc/dropbutton/dropbutton.base.css | misc/dropbutton/dropbutton.css |
modules/block/block.admin.css | modules/block/css/block.admin.css |
modules/book/book.admin.css | modules/book/css/book.admin.css |
modules/book/book.theme-rtl.css | modules/book/css/book.theme-rtl.css |
modules/book/book.theme.css | modules/book/css/book.theme.css |
modules/color/color.admin-rtl.css | modules/color/css/color.admin-rtl.css |
modules/color/color.admin.css | modules/color/css/color.admin.css |
modules/comment/comment.theme-rtl.css | modules/comment/css/comment.theme-rtl.css |
modules/comment/comment.theme.css | modules/comment/css/comment.theme.css |
modules/contextual/contextual.base.css | modules/contextual/css/contextual.module.css |
modules/contextual/contextual.theme-rtl.css | modules/contextual/css/contextual.theme-rtl.css |
modules/contextual/contextual.theme.css | modules/contextual/css/contextual.theme.css |
modules/contextual/contextual.toolbar-rtl.css | modules/contextual/css/contextual.toolbar-rtl.css |
modules/contextual/contextual.toolbar.css | modules/contextual/css/contextual.toolbar.css |
modules/dblog/dblog-rtl.css | modules/dblog/css/dblog.module-rtl.css |
modules/dblog/dblog.css | modules/dblog/css/dblog.module.css |
modules/edit/css/edit.css | modules/edit/css/edit.module.css |
modules/entity_reference/css/entity_reference-rtl.admin.css | modules/entity_reference/css/entity_reference.admin-rtl.css |
modules/field/theme/field-rtl.css | modules/field/css/field.module-rtl.css |
modules/field/theme/field.css | modules/field/css/field.module.css |
modules/field_ui/field_ui.admin-rtl.css | modules/field_ui/css/field_ui.admin-rtl.css |
modules/field_ui/field_ui.admin.css | modules/field_ui/css/field_ui.admin.css |
modules/file/file.admin.css | modules/file/css/file.admin.css |
modules/filter/filter.admin-rtl.css | modules/filter/css/filter.admin-rtl.css |
modules/filter/filter.admin.css | modules/filter/css/filter.admin.css |
modules/forum/forum-rtl.css | modules/forum/css/forum.module-rtl.css |
modules/forum/forum.css | modules/forum/css/forum.module.css |
modules/help/help-rtl.css | modules/help/css/help.module-rtl.css |
modules/help/help.css | modules/help/css/help.module.css |
modules/image/image.admin.css | modules/image/css/image.admin.css |
modules/image/image.theme-rtl.css | modules/image/css/image.theme-rtl.css |
modules/image/image.theme.css | modules/image/css/image.theme.css |
modules/language/language.admin.css | modules/language/css/language.admin.css |
modules/layout/layout.admin.css | modules/layout/css/layout.admin.css |
modules/locale/locale.admin-rtl.css | modules/locale/css/locale.admin-rtl.css |
modules/locale/locale.admin.css | modules/locale/css/locale.admin.css |
modules/menu/menu.admin.css | modules/menu/css/menu.admin.css |
modules/node/node.admin.css | modules/node/css/node.admin.css |
modules/node/node.edit.admin.css | modules/node/css/node.module.css |
modules/openid/openid-rtl.css | modules/openid/css/openid.module-rtl.css |
modules/openid/openid.css | modules/openid/css/openid.module.css |
modules/openid/login-bg.png | modules/openid/images/login-bg.png |
modules/overlay/overlay-child-rtl.css | modules/overlay/css/overlay-child-rtl.css |
modules/overlay/overlay-child.css | modules/overlay/css/overlay-child.css |
modules/overlay/overlay-parent.css | modules/overlay/css/overlay-parent.css |
modules/shortcut/shortcut.base-rtl.css | modules/shortcut/css/shortcut.module-rtl.css |
modules/shortcut/shortcut.base.css | modules/shortcut/css/shortcut.module.css |
modules/shortcut/shortcut.theme-rtl.css | modules/shortcut/css/shortcut.theme-rtl.css |
modules/shortcut/shortcut.theme.css | modules/shortcut/css/shortcut.theme.css |
modules/shortcut/shortcut.png | modules/shortcut/images/shortcut-add.png |
modules/simpletest/simpletest.css | modules/simpletest/css/simpletest.module.css |
modules/system/system.admin-rtl.css | modules/system/css/system.admin-rtl.css |
modules/system/system.admin.css | modules/system/css/system.admin.css |
modules/system/system.diff.css | modules/system/css/system.diff.css |
modules/system/system.maintenance.css | modules/system/css/system.maintenance.css |
modules/system/system.base-rtl.css | modules/system/css/system.module-rtl.css |
modules/system/system.base.css | modules/system/css/system.module.css |
modules/system/system.plugin.ui.css | modules/system/css/system.plugin.ui.css |
modules/system/system.theme-rtl.css | modules/system/css/system.theme-rtl.css |
modules/system/system.theme.css | modules/system/css/system.theme.css |
modules/system/tests/system.base.css | modules/system/tests/css/system.module.css |
modules/taxonomy/taxonomy.css | modules/taxonomy/css/taxonomy.module.css |
modules/toolbar/css/toolbar.base-rtl.css | modules/toolbar/css/toolbar.module-rtl.css |
modules/toolbar/css/toolbar.base.css | modules/toolbar/css/toolbar.module.css |
modules/tour/css/tour-rtl.css | modules/tour/css/tour.module-rtl.css |
modules/tour/css/tour.css | modules/tour/css/tour.module.css |
modules/translation_entity/translation_entity.admin.css | modules/translation_entity/css/translation_entity.admin.css |
modules/update/update-rtl.css | modules/update/css/update.admin-rtl.css |
modules/update/update.css | modules/update/css/update.admin.css |
modules/user/user-rtl.css | modules/user/css/user.module-rtl.css |
modules/user/user.css | modules/user/css/user.module.css |
modules/views/css/views.base.css | modules/views/css/views.module.css |
modules/views_ui/css/views-admin-rtl.css | modules/views_ui/css/views_ui.admin-rtl.css |
modules/views_ui/css/views-admin.css | modules/views_ui/css/views_ui.admin.css |
modules/views_ui/css/views-admin.theme-rtl.css | modules/views_ui/css/views_ui.admin.theme-rtl.css |
modules/views_ui/css/views-admin.theme.css | modules/views_ui/css/views_ui.admin.theme.css |
modules/views_ui/css/views-admin.contextual.css | modules/views_ui/css/views_ui.contextual.css |
This is part of the CSS standard described at http://drupal.org/node/1887922
Remaining tasks
Need to clarify naming convention, refer tim.plunkett query at http://drupal.org/node/1921610#comment-7362724 MODULE.module.css, MODULE.admin.css, MODULE.admin.theme.css and MODULE.theme.css have been agreed to.
User interface changes
none
API changes
Comment | File | Size | Author |
---|---|---|---|
#62 | 1987066-62.patch | 796 bytes | swentel |
#60 | Screen Shot 2013-06-07 at 14.57.44.png | 151.68 KB | swentel |
#55 | 1987066-55-rename-css-files.patch | 76.26 KB | echoz |
#51 | 1987066-51-rename-css-files.patch | 76.25 KB | echoz |
#49 | 1987066-49-rename-css-files.patch | 78.61 KB | echoz |
Comments
Comment #1
tim.plunkettWe decided to not introduce a new confusing term in 'skin', and continue to use the more Drupal-accurate 'theme'. Updated the issue summary.
Comment #2
Kevin Morse CreditAttribution: Kevin Morse commentedDo you want one patch to do this all?
Comment #3
Kevin Morse CreditAttribution: Kevin Morse commentedHere's one that does book, edit, field, forum, image, node, openid, shortcut, simple_test, tour
comment
contextual
help
taxonomy
update
user
Are missing still but I am working on them.
Comment #4
tim.plunkettWe're not using 'skin' anymore.
Also, this doesn't look like its renaming files at all, just adding new ones.
Comment #5
Kevin Morse CreditAttribution: Kevin Morse commentedOkay this should do it.
All of the modules are updated. The only thing I didn't quite understand is
Thanks,
Kevin
Comment #6
Kevin Morse CreditAttribution: Kevin Morse commentedWoops forgot to change status.
Comment #7
Kevin Morse CreditAttribution: Kevin Morse commentedAny way to cancel tests that don't need to be run? The only one that needs to run is the last one. Sorry for the extras.
Comment #8
rteijeiro CreditAttribution: rteijeiro commentedKevin, just add -do-not-test to the patch name, for example: 1987066-rename-css-3-do-not-test.patch
And the testbot will not test this patch.
Comment #9
tim.plunkettWe must have no test coverage for this. The file added is the wrong name.
Otherwise it looks good, thanks @Kevin Morse.
If you need a test cancelled, ask in #drupal-contribute, there are several qa.d.o users with access to do that (myself included).
Comment #10
Kevin Morse CreditAttribution: Kevin Morse commentedOkay this should fix that mistake.
Comment #11
dealancer CreditAttribution: dealancer commentedGood idea those patch also covers issues, that were already done some time ago but were closed as duplicates: http://drupal.org/project/issues/search/drupal?issue_tags=d8mux-css-cleanup.
Comment #12
jwilson3I object.
I've watched the arguments for/against "skin" change, and am happy to see that things are staying as "theme". Noone seems to have questioned the reasoning or bothered to explain in the issue summary why the change from "base" to "module".
Note that we are also proposing a new "drupal.base.css" in #1924430: Add drupal.base library for base CSS styles and with this issue all other modules' "base" css is being converted to "modulename.module.css". We will have:
drupal.base.css
node.module.css
node.admin.css
node.admin.theme.css
node.theme.css
So, why have this special case naming convention *just* for drupal.base.css?
I like "base". It's not that complicated for both back-end module devs or front-end themers to understand the concept of a "base" stylesheet -- whether it pertains to Drupal as a whole, or a specific module or theme -- particularly when the "thing" that it is a base of is right there in the file's name.
Additionally, the term "base" inherently induces and reinforces the existing concept of hierarchy and makes it plainly obvious which stylesheet of the 3 or four that a module provides is loaded first on the page. And anyone who understands the concept of stylesheet rule order and inheritance will understand that a "drupal.base.css" will come before any other "modulename.base.css", so I dont think there is any reason to think that using "base" for modules would pollute the namespace used for the Drupal base stylesheet.
This seems like its introducing more complexity rather than clarity, but maybe thats just me.
I would propose we leave "base" stylesheets just as they are, which will allow the theming layer to not be tied to the term "module" and could open the way for *components*, *themes*, *plugins*, *distros* etc to have their own base stylesheet.
Comment #13
Kevin Morse CreditAttribution: Kevin Morse commented@jwilson3 I agree... However, I'm not about to go out and redo that patch without some consensus.
Comment #14
Kevin Morse CreditAttribution: Kevin Morse commented#10: 1987066-rename-css-10.patch queued for re-testing.
Comment #15
jwilson3I just re-read:
https://smacss.com/book/type-base
and
https://smacss.com/book/type-module
(it had been awhile).
And now I understand from where my confusion was stemming. The problem boils down to the fact that Drupal uses the term module to mean something completely different than SMACSS' meaning.
Drupal module: a grouping of backend and possibly front end code and web assets that works together to provide a specific feature on a website.
SMACSS module: a single discrete component (read: chunk of html) on the page.
A Drupal module, could therefore contain many many SMACSS' modules inside it, each default theme hook implementation could be a separate SMACSS module. Using the Node module as an example, the Recent Content Block, the Node Search Admin, and each view-mode could represent a SMACSS module, requiring different component-level stylesheet rules.
I expect that I won't be the first person to misinterpret this.
IMHO, adopting this naming convention for css files pollutes the Drupal terminology namespace for "modules", and creates a WTF, because other components, including themes, install profiles, etc, could all conceivably contain a css file called X.module.css in order to follow the SMACSS convention.
Comment #16
Kevin Morse CreditAttribution: Kevin Morse commented#10: 1987066-rename-css-10.patch queued for re-testing.
Comment #18
Kevin Morse CreditAttribution: Kevin Morse commentedIs there any point in re-rolling this patch? Who has the final say regarding module.css vs. base.css?
http://drupal.org/node/1921610#comment-7413396 seems to have made up its mind?
Comment #19
jwilson3If I understand correctly No need to keep re-rolling this, per Comment #36 on #1921610-36: [Meta] Architect our CSS.
it looks like this is being done in a sandbox for the time being.
Comment #20
JohnAlbinI'm uploading the patch we have from the 8.x-css branch in the sandbox. Let's see what the testbot says.
There is a GIGANTIC list of contributors (many 1st timer core contributors) to this patch. I'll list them if the testbot says its green.
Comment #22
rteijeiro CreditAttribution: rteijeiro commented#20: 1987066-20-rename-css-files.patch queued for re-testing.
Comment #23
JohnAlbinHuh. trying again.
Comment #23.0
JohnAlbinclarified that we're ditching 'skin'
Comment #23.1
JohnAlbinRemove todo
Comment #24
JohnAlbinThe proposal is not a SMACSS naming convention for Drupal CSS files. *.module.css and *.theme.css refer to the Drupal concepts, not the SMACSS concepts. We want to rename the current *.base.css files because "base" isn’t a known Drupal term and it conflicts with SMACSS’ "base" concept.
Just to clarify, this is making our current CSS file naming 100% consistent across all core modules. We currently have a BAT system that is inconsistently implemented. After we make this change, we still need to componentize all of Drupal core's CSS per #1921610: [Meta] Architect our CSS which should break down these monolithic CSS files into smaller chunks. But, first things first; let's fix this crap now. :-)
When this gets committed, here's the commit message which includes all the contributors in the sandbox. Many of them are first time contributors that attended DrupalCamp Singapore. :-)
Comment #25
ry5n CreditAttribution: ry5n commented@jwilson3 Adding to what @JohnAlbin said in #24: “module” will always refer to a Drupal module. When talking about SMACSS in Drupal, the consensus is to use the term “component” to refer to a SMACSS module.
I’ve gone through #20 line-by-line but haven’t tested the patch. A bit gruelling so I could have missed things. Needs more eyeballs, and testing.
The only thing that really sticks out is the existence of files outside the module/theme/admin/admin.theme scheme, e.g. system.maintenance.css, system.plugin.ui.css. Are they an issue?
Both node.admin.css and node.edit.admin.css are admin styles, but they are conditionally loaded in different situations. Neither is used for public pages, so renaming one to node.module doesn’t seem right here. Perhaps node.admin.css → node.admin.revisions.css and node.edit.admin.css → node.admin.node-form.css?
Nitpick: can we take the opportunity to dasherize this file name?
Comment #26
echoz CreditAttribution: echoz commented#20 patch results in error: patch failed: core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.php
I edited that file updating the paths and file names, but it still stumped me at the top one ~ line 46.
Comment #27
rteijeiro CreditAttribution: rteijeiro commentedYes, I am looking into it. It seems needing rebase against the latest changes of the 8.x branch.
Comment #28
JohnAlbinI've resolved the merge conflict that occurs when pulling in the latest 8.x branch.
Comment #29
JohnAlbinWhoops! Wrong patch file!
Comment #30
echoz CreditAttribution: echoz commentedcode review good, but we missed some.
toolbar.base.css
views.base.css
dropbutton.base.css (under misc)
Comment #31
JohnAlbinIn addition to echoz's comments, I noticed that views_ui module doesn't follow the MODULE_NAME.thingie.css standard all our other modules use. So I've change its views-admin.* files to views_ui.*
New patch!
Comment #32
echoz CreditAttribution: echoz commentedcode + source look good.
Comment #32.0
echoz CreditAttribution: echoz commentedRemove cruft about "theme" from issue summary that is no longer relevant.
Comment #32.1
JohnAlbinAdd full list of files that are moved or renamed.
Comment #33
JohnAlbinI've updated the issue summary to provide a full list of CSS files that have been moved/renamed in the patch in #31.
The patch was generated from the 8.x-css branch of the Drupal 8 mobile initiative sandbox: http://drupal.org/sandbox/johnalbin/1488942
Comment #34
ry5n CreditAttribution: ry5n commentedI’m doing manual testing and checking the browser console for 404s. Looks like there are still some paths to update (overlay and dblog modules so far). Working on a patch as I go.
Comment #35
ry5n CreditAttribution: ry5n commentedWent through as much of the admin UI and Bartik as I could. Seems like it’s just those paths in overlay and dblog modules that needed updating.
I rolled this on a local 8.x branch, so I guess this would need to be applied to the Mobile sandbox as well.
Comment #37
echoz CreditAttribution: echoz commented#35: 1987066-35-rename-css-files.patch queued for re-testing.
Comment #39
ry5n CreditAttribution: ry5n commented#35: 1987066-35-rename-css-files.patch queued for re-testing.
Comment #40
rteijeiro CreditAttribution: rteijeiro commentedPatch applied on the latest 8.x and Mobile sandbox and applies well.
Checked for 404 errors and everything seems ok. Ready for RTBC or should I check other things?
Comment #41
ry5n CreditAttribution: ry5n commented@rteijeiro We should probably test this in Stark as well. If you have time, you could do a run-through of Bartik and Stark as admin themes. After that, if we see no issues I’d say RTBC.
Comment #42
rteijeiro CreditAttribution: rteijeiro commentedRerolled the patch to apply with the latest 8.x changes. There were conflicts with core/modules/help/lib/Drupal/help/Controller/HelpController.php and core/modules/block/block.admin.inc files.
Checked with Bartik and Stark also as Admin themes. No 404 errors found.
Comment #43
echoz CreditAttribution: echoz commented@rteijeiro, your blank lines have a whitespace. It's really helpful to set your text editor to strip whitespace.
Comment #44
rteijeiro CreditAttribution: rteijeiro commentedHi echoz,
I didn't find whitespaces in my changes. Please, let me know where did you find them an I will reroll the patch. Also I have noticed that the core/modules/help/lib/Drupal/help/Controller/HelpController.php file is not in the previous patch #42 (something wrong happened).
Comment #45
rteijeiro CreditAttribution: rteijeiro commentedI guess I fixed the whitespaces problem and also included the changes of the core/modules/help/lib/Drupal/help/Controller/HelpController.php file.
Comment #46
echoz CreditAttribution: echoz commented/help.css is changed to /css/help.module.css
this patch left out ".module" there.
Comment #47
rteijeiro CreditAttribution: rteijeiro commentedSorry, I forgot the file name :(
Hope now it's right. Let's see what says the testbot.
Comment #49
echoz CreditAttribution: echoz commentedI found something that was originally missed. The *toolbar* shortcut icon image was mistakenly referenced also for the shortcut add/remove icon, which was named the same and not in the images folder, it was with the other module files. I've renamed and moved the add shortcut icon into the images folder and adjusted the css to point to it.
Also cleaned up 2 places with double blank lines.
Comment #51
echoz CreditAttribution: echoz commentedopenid got removed from core, re-roll.
Comment #52
rteijeiro CreditAttribution: rteijeiro commentedThanks @echoz. Now applies well. Checked and no 404 errors found. Maybe RTBC finally?
Comment #53
echoz CreditAttribution: echoz commented@rteijeiro, if you've done your typical testing of this (perhaps as ry5n suggested in #41), it should be ok.
Comment #54
rteijeiro CreditAttribution: rteijeiro commented@echoz, forgot to comment that, sorry. Checked with Stark, Seven and Bartik as default and admin themes. No 404 errors.
Marked as RTBC. Feel free to change it if you notice some errors.
Comment #55
echoz CreditAttribution: echoz commentedLots of commit action in core, re-roll. Just line #'s changed. Let's get this committed!
Comment #56
JohnAlbin> Just line #'s changed
If its just line numbers, it doesn't need a reroll. That just knocks the patch out of the list of issues the committers are looking at.
Comment #57
echoz CreditAttribution: echoz commentedcrap, I was just trying to answer that for myself, but too late.
Comment #58
JohnAlbinBack to RTBC. :-)
Comment #59
alexpottCommitted 2432c02 and pushed to 8.x. Thanks!
Comment #60
swentel CreditAttribution: swentel commentedHmm, simpletest looks so white now ...
Comment #61
swentel CreditAttribution: swentel commentedComment #62
swentel CreditAttribution: swentel commentedForgot one ..
Comment #63
rteijeiro CreditAttribution: rteijeiro commentedNice catch @swentel, thanks!! :)
Comment #64
yched CreditAttribution: yched commentedIndeed, it's quite impossible to work on tests right now :-)
#62 fixes it fine.
Quick commit ?
Comment #65
alexpottCommitted ba51ddf and pushed to 8.x. Thanks!
Guess it shows that I never run tests in the ui :)
Comment #66
JohnAlbinOh, doh! Looks like credits only went to Kevin Morse, JohnAlbin, rteijeiro, echoz, ry5n. I should have re-posted the credit from comment #24 again since this patch was the combined effort of many mini-issues in the D8 mobile sandbox.
All of these people helped with the combined patch for this issue. And they are all awesome! Thank you all!
Comment #66.0
JohnAlbinUpdate CSS file list
Comment #67
YesCT CreditAttribution: YesCT commentedhttps://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.
Comment #68
Shyamala CreditAttribution: Shyamala commentedI am going to try to write the change notice.
Comment #69
Shyamala CreditAttribution: Shyamala commentedassigning to myself
Comment #70
Shyamala CreditAttribution: Shyamala commentedSummary
As part of Drupal 8 Mobile Initiative the entire Drupal CSS is getting re-architected based on SMACSS. Refer https://drupal.org/node/1886770 for details on the new architecture.
One of the tasks of this Architect CSS effort is to rename and organise the CSS files. CSS rulesets are grouped into logical files, that can be aggregated efficiently and that can be easily overridden by themers.
CSS files for Drupal 8 modules, Renaming tasks included:
As a part of this issue All Drupal module’s CSS files are renamed to match new file naming convention
eg. MODULE.base.css becomes MODULE.module.css. In addition, CSS files are moved to the css subdirectory.
For details on CSS file organization in Drupal 8 refer: https://drupal.org/node/1887922
Comment #71
JohnAlbinLooks good to me! Go ahead and make the change notice with that language, Shyamala.
Comment #72
Shyamala CreditAttribution: Shyamala commentedChange record created at https://drupal.org/node/2016305
Comment #73.0
(not verified) CreditAttribution: commentedadded shortcut-add.png rename/move explained in #49