Problem/Motivation
Part of #1921610: [Meta] Architect our CSS
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 Forum module does not yet follow the guidelines.
Proposed resolution
forum.css into css/forum.skin.css
forum-rtl.css into css/forum.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
Test if with this patch the CSS is being added to Drupal from its new location with its new name
User interface changes
none
API changes
The forum.module's CSS files will have new names.
By akmalfikri and Gomez_in_the_South
Comment | File | Size | Author |
---|---|---|---|
#15 | 1981036-forum-css.patch | 1.27 KB | akmalfikri |
#12 | 1981036-forum-css-icon-fix.patch | 3.89 KB | akmalfikri |
#11 | after.png | 2.96 KB | dcam |
#2 | 1981036-forum-css-renaming-into-correct-naming-convention.patch | 3.89 KB | akmalfikri |
#1 | 1981036-rename-forum-css-into-the-naming-convention.patch | 3.88 KB | akmalfikri |
Comments
Comment #1
akmalfikri CreditAttribution: akmalfikri commentedHere's the patch
Comment #2
akmalfikri CreditAttribution: akmalfikri commentedThe correct patch
Comment #3
Gomez_in_the_South CreditAttribution: Gomez_in_the_South commentedI reviewed the patch from #2 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 #4
echoz CreditAttribution: echoz commentedComment #5
akmalfikri CreditAttribution: akmalfikri commentedIgnore #1
Comment #6
akmalfikri CreditAttribution: akmalfikri commentedAdding tags
Comment #7
Gomez_in_the_South CreditAttribution: Gomez_in_the_South commentedReviewed and tested.
Comment #8
Shyamala CreditAttribution: Shyamala commentedtagging
Comment #9
Shyamala CreditAttribution: Shyamala commentedThanks @gomez_in_the_south! Can you confirm what you reviewed, refer: http://drupal.org/node/1489010.
Please read http://drupal.org/node/1489010 for details on Manual testing.
Comment #10
Shyamala CreditAttribution: Shyamala commented@Gomez_in_the_South thanks for the testing!
Can you confirm what you reviewed, refer: http://drupal.org/node/1489010.
Please read http://drupal.org/node/1489010 for details on Manual testing.
check john's review at http://drupal.org/node/1981026#comment-7352628
Changing status to needs review
Comment #11
dcam CreditAttribution: dcam commented#2 needs work. The forum-icons.png URL needs to be updated.
Otherwise, the patch looks good. I didn't find any additional uses of the old CSS file names. The moved CSS files in the /css directory are being applied to the forum pages.
Comment #12
akmalfikri CreditAttribution: akmalfikri commentedMy bad. Here's the latest patch.
Comment #13
aspilicious CreditAttribution: aspilicious commentedI believe the standard is ti put all the cs sin alphabetical order (except for some exceptions for vender prefixes). Can we do that here?
Comment #14
aspilicious CreditAttribution: aspilicious commentedApparantly that rule is gone in the new standards :). So forget my comment.
http://drupal.org/node/1887862#declaration-order
Comment #15
akmalfikri CreditAttribution: akmalfikri commentedHi guys,
Apparently my git does not comply with the git config stated here : http://drupal.org/documentation/git/configure
After applied the new git config, here is the rerolled patch. I hope it's the correct one.
Comment #17
akmalfikri CreditAttribution: akmalfikri commented#15: 1981036-forum-css.patch queued for re-testing.
Comment #18
dcam CreditAttribution: dcam commented#15 looks good. It incorporates the change from #11 and fixes the file renaming issue.
Comment #19
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. Thanks everyone on this issue, looking to your continued participation in the new issue.
Refer: http://drupal.org/node/1921610#comment-7375894
Comment #20
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. (per webchick's request)
So you're work is not lost! I'm moving this issue to the Mobile Initiative sandbox. :-)
Comment #21
mtiftCommitted to the sandbox! :-)
Comment #22.0
(not verified) CreditAttribution: commentedAdded steps for testing