Problem/Motivation

Our CSS standards recommend that CSS is split up into smaller files inline with these categories:

  • Base
  • Layout
  • Component
  • Theme

Proposed resolution

Let's split up Sevens style.css into these categories. Let's not fix any problems we fix while moving files.

Remaining tasks

- Review the patch (code). Keep in mind that you should have some knowledge about SMACSS in order to review this patch. Are the files separated correctly?
- Manually review the patch. Does everything look the same when the patch is applied, since only files are moved?
- RTBC!

User interface changes

None

API changes

None

CommentFileSizeAuthor
#19 Screenshot 2014-09-09 12.33.46.jpg504.27 KBLewisNyman
#18 Performance-Drupal8-no-aggregate.png92.43 KBjibran
#18 Performance-Drupal8-aggregate.png91.18 KBjibran
#17 Screenshot 2014-09-09 12.14.45.jpg319.58 KBLewisNyman
#16 Drupal8-before.png90.88 KBjibran
#16 Drupal8-now.png90.22 KBjibran
#11 2321505-split-seven-css-11.patch84.44 KBLewisNyman
#11 interdiff.txt2.11 KBLewisNyman
#7 2321505-split-seven-css-5.patch84.64 KBLewisNyman
#7 interdiff.txt1.96 KBLewisNyman
#6 2321505-split-seven-css-4.patch84.46 KBLewisNyman
#5 2321505-split-seven-css-3.patch84.46 KBLewisNyman
#5 2321505-split-seven-css-3.patch84.46 KBLewisNyman
#1 2321505-split-seven-css-1.patch77.74 KBLewisNyman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
2321505-split-seven-css-1.patch77.74 KB

Here's the patch. Image url paths have been updated. I'm not sure how to make this easier to review.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2321505-split-seven-css-1.patch, failed testing.

sqndr’s picture

sqndr CreditAttribution: sqndr commented
Issue summary: View changes
Issue tags: +Needs tests

Adding Needs tests tag, since the tests require an update.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2321505-split-seven-css-3.patch84.46 KB
2321505-split-seven-css-3.patch84.46 KB

Fixing tests and removing install CSS from every page.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
2321505-split-seven-css-4.patch84.46 KB

Just realised I named the breadcrumb.css incorrectly

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
interdiff.txt1.96 KB
2321505-split-seven-css-5.patch84.64 KB

Sorry, a few missing sheets.

The last submitted patch, 5: 2321505-split-seven-css-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2321505-split-seven-css-5.patch, failed testing.

The last submitted patch, 6: 2321505-split-seven-css-4.patch, failed testing.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.11 KB
2321505-split-seven-css-11.patch84.44 KB

Fixing tests

sqndr’s picture

sqndr CreditAttribution: sqndr commented
Issue summary: View changes

Updated the summary.

sqndr’s picture

sqndr CreditAttribution: sqndr commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Would love to get this in to start documenting the css.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/system/src/Tests/Menu/MenuRouterTest.php
@@ -266,7 +266,7 @@ public function testThemeIntegration() {
-    $this->assertRaw('seven/css/style.css', "The administrative theme's CSS appears on the page.");
+    $this->assertRaw('seven/css/base/elements.css', "The administrative theme's CSS appears on the page.");

@@ -285,7 +285,7 @@ protected function doTestThemeCallbackMaintenanceMode() {
-    $this->assertRaw('seven/css/style.css', "The administrative theme's CSS appears on the page.");
+    $this->assertRaw('seven/css/base/elements.css', "The administrative theme's CSS appears on the page.");

+++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
@@ -166,13 +166,36 @@ public function testRebuildThemeData() {
-        'css/seven.base.css' => DRUPAL_ROOT . '/core/themes/seven/css/seven.base.css',
-        'css/style.css' => DRUPAL_ROOT . '/core/themes/seven/css/style.css',
-        'css/layout.css' => DRUPAL_ROOT . '/core/themes/seven/css/layout.css',
+        'css/base/elements.css' => DRUPAL_ROOT . '/core/themes/seven/css/base/elements.css',

Interesting that our underlying system tests test stuff in Seven theme, but that's a pre-existing condition. :)

I looked through this pretty carefully and it all seems to be affecting things only in Seven theme, not in any other CSS in core, so should be safe. It's also been RTBC for about a week, which should be ample time for people to raise concerns if there are any.

Committed and pushed to 8.x. Thanks!

  • webchick committed 32336d5 on 8.0.x 
    Issue #2321505 by LewisNyman: Split Seven's style.css into SMACSS...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Category: Task » Bug report
Status: Fixed » Active
FileSize
Drupal8-now.png90.22 KB
Drupal8-before.png90.88 KB

Sorry it added a CSS regression.

D8 now

D8 before

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
Screenshot 2014-09-09 12.14.45.jpg319.58 KB

Did you try a reinstall? I see this on a fresh install:

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
Performance-Drupal8-aggregate.png91.18 KB
Performance-Drupal8-no-aggregate.png92.43 KB

it is fine with aggregate but not without it.

Aggregate on

Aggregate off

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
Screenshot 2014-09-09 12.33.46.jpg504.27 KB

hmm that's weird because mine was without aggregate as well.

Here's a shot from Simplytest.me

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Category: Bug report » Task
Status: Active » Fixed

NVM it was ABP of my chrome which doesn't allow me to load branding.css see http://stackoverflow.com/questions/23341765/getting-neterr-blocked-by-cl...
It is working fine in firefox and after disabling ABP in chrome.
Sorry for the noise and thank you for your patience @LewisNyman

Status: Fixed » Closed (fixed)

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