Problem/Motivation

Meta Issue: #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 Shortcut module does not yet follow the guidelines.

Proposed resolution

shortcut.base.css into css/shortcut.module.css
shortcut.base-rtl.css into css/shortcut.module-rtl.css
shortcut.theme-rtl.css into css/shortcut.skin-rtl.css
shortcut.theme.css into css/shortcut.skin.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

Needs manual testing - Test 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 shortcut.module's CSS files will have new names.

By akmalfikri and Gomez_in_the_South

Comments

akmalfikri’s picture

Status: Active » Needs review
StatusFileSize
new7.93 KB

Here's the patch

gomez_in_the_south’s picture

Status: Needs review » Active

I reviewed the patch in #1 and the CSS is being added to Drupal from its new location with its new name. It is RTBC from me, waiting for testbot to complete review.

johnalbin’s picture

Title: Rename shorcut module CSS files to match new file naming convention » Rename shortcut module CSS files to match new file naming convention
Status: Active » Needs review
shyamala’s picture

Issue tags: +mobile, +d8mux, +d8mux-css-cleanup

Tagging

shyamala’s picture

Issue tags: +Novice

adding novice tag

shyamala’s picture

Project: Drupal core » Drupal 8 Mobile Initiative
Issue summary: View changes

Changed the naming convention

gomez_in_the_south’s picture

Project: Drupal 8 Mobile Initiative » Drupal core
Status: Needs review » Reviewed & tested by the community

Reviewed and tested.

shyamala’s picture

Issue tags: +Needs manual testing

@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

shyamala’s picture

Status: Reviewed & tested by the community » Needs review

changing status to needs review

dcam’s picture

Status: Needs review » Needs work
StatusFileSize
new4.19 KB

#1 needs work. The image URLs were not updated, e.g. changing "images/" to "../images/".

+++ b/core/modules/shortcut/css/shortcut.skin.cssundefined
@@ -0,0 +1,52 @@
+.icon-shortcut:before {
+  background-image: url("images/shortcut.png");
+}
+.icon-shortcut:active:before,
+.icon-shortcut.active:before {
+  background-image: url("images/shortcut-active.png");
+}
+++ b/core/modules/shortcut/css/shortcut.skin.cssundefined
@@ -0,0 +1,52 @@
+.add-or-remove-shortcuts .icon {
+  background: transparent url(shortcut.png) no-repeat;
+  height: 12px;
+  margin-left: 8px; /* LTR */
+  overflow: hidden;
+  text-indent: 12px;
+  width: 12px;
+}

Otherwise, the patch looks good. I found no additional usages of the old CSS file names. The moved files are correctly being added to pages from their new /css directory.

after.png

akmalfikri’s picture

Status: Needs work » Needs review
StatusFileSize
new7.94 KB

My bad.

Here's the patch for #9

Status: Needs review » Needs work
Issue tags: -Novice, -mobile, -Needs manual testing, -d8mux, -d8mux-css-cleanup

The last submitted patch, 1981054-shortcut-css-image-fix.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +mobile, +Needs manual testing, +d8mux, +d8mux-css-cleanup
tim.plunkett’s picture

Status: Needs review » Needs work

Per #1921610-18: [Meta] Architect our CSS and further pushback against "skin", please do not rename .theme.css files to .skin.css.

akmalfikri’s picture

Status: Needs work » Needs review
StatusFileSize
new2.98 KB

Here's the latest patch.

I have put the latest patch with the following amendments :

1. The correct git config as stated here : http://drupal.org/documentation/git/configure
2. Changed back .skin.css into .theme.css

Hope this is the correct one.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I checked #14. It incorporates the changes from #9 and #13. I wondered what the deal was with the file deleting/adding instead of renaming. Anyway, #14 Looks good to me.

shyamala’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

Created 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

johnalbin’s picture

Project: Drupal core » Drupal 8 Mobile Initiative
Version: 8.x-dev »
Component: CSS » CSS architecture
Status: Closed (duplicate) » Reviewed & tested by the community

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

johnalbin’s picture

Status: Reviewed & tested by the community » Fixed

Committed to the sandbox! :-)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added steps for manual testing