Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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" and "theme" means something else in SMACSS. So we just need to rename them.
The Book module does not yet follow the guidelines.
Proposed resolution
Rename book/book.admin.css into book/css/book.admin.css
Rename book/book.theme.css into book/css/book.skin.css
Rename book/book.theme-rtl.css into book/css/book.skin-rtl.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 do that.
This is part of the CSS standard described at http://drupal.org/node/1887922
Remaining tasks
This issue is blocked by #1924436: Remove separate CSS_AGGREGATE_SYSTEM aggregate file and fix drupal_add_library() to aggregate CSS properly
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
The book.module's CSS files will have new names.
By akmalfikri and Gomez_in_the_South
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | d8-mobile-rename-module-css-book-20.patch | 1.58 KB | mtift |
| #18 | d8-mobile-rename-module-css-book.patch | 613 bytes | carwin |
| #9 | 1981022-book-css-reroll-2.patch | 1.58 KB | akmalfikri |
| #8 | 1981022-book-css-reroll.patch | 4.56 KB | akmalfikri |
| #7 | 1981022-book-css.patch | 5.6 KB | akmalfikri |
Comments
Comment #0.0
akmalfikri CreditAttribution: akmalfikri commentedAdded contributor
Comment #1
akmalfikri CreditAttribution: akmalfikri commentedHere's the patch
Comment #2
JohnAlbinI reviewed this patch and the CSS is being added to Drupal from its new location with its new name. It is RTBC from me, but I'll let the testbot review first. :-)
Comment #3
JohnAlbinHuzzah!
Comment #3.0
JohnAlbinChanged the new css file name
Comment #4
Shyamala CreditAttribution: Shyamala commentedTagging
Comment #5
alexpottWe need to re-roll the patch so that git recognises that these are just file moves... @akmalfikri see https://drupal.org/documentation/git/configure... especially the part on Optimize diffs for renamed and copied files
Comment #5.0
alexpottUpdated issue summary.
Comment #6
alexpottD.O I did not remove that tag :)
Comment #7
akmalfikri CreditAttribution: akmalfikri commentedHi,
Here's the rerolled patch with the correct configuration stated here : http://drupal.org/documentation/git/configure
Comment #8
akmalfikri CreditAttribution: akmalfikri commentedAccidentally added shorcut module's code with the patch. Here's the correct one.
UPDATE :
Ignore this one. Apparently there is something wrong with my branching
Comment #9
akmalfikri CreditAttribution: akmalfikri commentedHi,
After fiddling around with my branches, finally I get it correctly, I hope.
Comment #10
dcam CreditAttribution: dcam commented#9 looks good. The patch was rerolled and is correctly renaming the files instead of deleting/adding. I found no instances of the old file locations still being used in code.
Comment #11
Shyamala CreditAttribution: Shyamala commentedCreated a single issue to rename all css files at: #1987066: Rename files to match CSS file naming convention based on request by webchick to make review easier. Refer: http://drupal.org/node/1921610#comment-7375894
Comment #12
Shyamala CreditAttribution: Shyamala commentedComment #13
Shyamala CreditAttribution: Shyamala commentedComment #14
JohnAlbinSorry for the delay in reviewing these patches. My bronchitis flared up and I've been too sick until this week to get back into the issue queue.
Lots of discussions have happened in the interim. We just held a D8 Mobile Initiative meeting on Google+: https://plus.google.com/u/1/events/c0knva4lgh4vot0nun5lbfel9fc where we decided that we could make the CSS re-archicture work move faster by moving the work into a sandbox git repository. Then we could commit lots of little issues to the sandbox and roll larger, more-complete patches into Drupal 8’s issue queue.
So you're work is not lost! I'm moving this issue to the Mobile Initiative sandbox. :-)
Comment #15
mtiftPer #1921610-18: [Meta] Architect our CSS and further pushback against "skin", we are not renaming .theme.css files to .skin.css.
Comment #16
carwin CreditAttribution: carwin commentedComment #17
mtift[EDIT: mistakenly marked fixed]
Comment #18
carwin CreditAttribution: carwin commentedAttaching patch that simply renames the css files and moves them into a 'css' directory.
Comment #19
carwin CreditAttribution: carwin commentedComment #20
mtiftActually, I think we still need those file path changes
Comment #21
dcam CreditAttribution: dcam commentedWhat's going on in this issue? #17 says this was fixed. Was this committed to the sandbox?
If not, #20 is the same as #9, which I RTBC'ed earlier. If it's not committed, #20 is RTBC.
Comment #22
carwin CreditAttribution: carwin commented@dcam - The issue got repurposed, our goal right now is JUST renaming the files, not actually updating the CSS. That said, I have no idea if mtift actually committed to the sandbox yet.
Comment #23
dcam CreditAttribution: dcam commentedGranted, I haven't been involved in the initiative - I only followed the issue here from the core queue, but it seems to me that not breaking the URLs in these files is pretty important. It has nothing to do with trying to change things about the way the CSS is structured. It's just preventing a regression.
Comment #24
mtiftNow it's committed to the sandbox! :-)
Comment #25.0
(not verified) CreditAttribution: commentedUpdated issue summary.