Rename files to match CSS file naming convention

We decided to not introduce a new confusing term in 'skin', and continue to use the more Drupal-accurate 'theme'. Updated the issue summary.

Do you want one patch to do this all?

Here'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.

We're not using 'skin' anymore.
Also, this doesn't look like its renaming files at all, just adding new ones.

Okay this should do it.

All of the modules are updated. The only thing I didn't quite understand is

but requires adding a MODULE.admin.theme.css so admin themes can override the default theme

Thanks,

Kevin

Woops forgot to change status.

Any 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.

Kevin, 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.

+++ b/core/modules/field/field.moduleundefined
index 5ac5e75..0c4f8fc 100644
--- a/core/modules/forum/forum.css

--- a/core/modules/forum/forum.css
+++ b/core/modules/forum/css/forum.module.cssundefined

+++ b/core/modules/forum/forum.pages.incundefined
@@ -73,6 +73,6 @@ function forum_page($forum_term = NULL) {
-  $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/forum.css';
+  $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/css/forum.css';

We 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).

Okay this should fix that mistake.

Good 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.

I 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.

@jwilson3 I agree... However, I'm not about to go out and redo that patch without some consensus.

#10: 1987066-rename-css-10.patch queued for re-testing.

I 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.

#10: 1987066-rename-css-10.patch queued for re-testing.

Is 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?

If 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.

I'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.

#20: 1987066-20-rename-css-files.patch queued for re-testing.

failed to checkout from git

Huh. trying again.

clarified that we're ditching 'skin'

Remove todo

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.

The 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. :-)

Issue #1987066 by akmalfikri, carwin, chaz.chumley, ckrina, criticny, dcam, dealancer, echoz, Gomez_in_the_South, j0rd, jchin1968, JohnAlbin, Kevin Morse, kim.pepper, mtift, nick_schuch, rteijeiro and svenryen: Rename files to match CSS file naming convention

@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?

+++ b/core/modules/menu/menu.admin.incundefined
diff --git a/core/modules/node/node.edit.admin.css b/core/modules/node/css/node.module.css
rename from core/modules/node/node.edit.admin.css
rename to core/modules/node/css/node.module.css

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?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -362,7 +362,7 @@ function translation_entity_library_info() {
+      $path . '/css/translation_entity.admin.css' => array(),

Nitpick: can we take the opportunity to dasherize this file name?

#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.

Yes, I am looking into it. It seems needing rebase against the latest changes of the 8.x branch.

I've resolved the merge conflict that occurs when pulling in the latest 8.x branch.

Whoops! Wrong patch file!

code review good, but we missed some.
toolbar.base.css
views.base.css
dropbutton.base.css (under misc)

In 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!

code + source look good.

Remove cruft about "theme" from issue summary that is no longer relevant.

Add full list of files that are moved or renamed.

I'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

I’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.

Went 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.

#35: 1987066-35-rename-css-files.patch queued for re-testing.

#35: 1987066-35-rename-css-files.patch queued for re-testing.

Patch 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?

@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.

Rerolled 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.

@rteijeiro, your blank lines have a whitespace. It's really helpful to set your text editor to strip whitespace.

Hi 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).

I guess I fixed the whitespaces problem and also included the changes of the core/modules/help/lib/Drupal/help/Controller/HelpController.php file.

/help.css is changed to /css/help.module.css

this patch left out ".module" there.

Sorry, I forgot the file name :(

Hope now it's right. Let's see what says the testbot.

I 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.

openid got removed from core, re-roll.

Thanks @echoz. Now applies well. Checked and no 404 errors found. Maybe RTBC finally?

@rteijeiro, if you've done your typical testing of this (perhaps as ry5n suggested in #41), it should be ok.

@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.

Lots of commit action in core, re-roll. Just line #'s changed. Let's get this committed!

> 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.

crap, I was just trying to answer that for myself, but too late.

Back to RTBC. :-)

Committed 2432c02 and pushed to 8.x. Thanks!

Hmm, simpletest looks so white now ...

Forgot one ..

Nice catch @swentel, thanks!! :)

Indeed, it's quite impossible to work on tests right now :-)
#62 fixes it fine.

Quick commit ?

Committed ba51ddf and pushed to 8.x. Thanks!

Guess it shows that I never run tests in the ui :)

Oh, 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.

Issue #1987066 by akmalfikri, carwin, chaz.chumley, ckrina, criticny, dcam, dealancer, echoz, Gomez_in_the_South, j0rd, jchin1968, JohnAlbin, Kevin Morse, kim.pepper, mtift, nick_schuch, rteijeiro and svenryen: Rename files to match CSS file naming convention

All of these people helped with the combined patch for this issue. And they are all awesome! Thank you all!

Update CSS file list

https://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.

Looks good to me! Go ahead and make the change notice with that language, Shyamala.