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

Comments

lewisnyman’s picture

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

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

Issue summary: View changes
Issue tags: +Needs tests

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

lewisnyman’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new84.46 KB
new84.46 KB

Fixing tests and removing install CSS from every page.

lewisnyman’s picture

StatusFileSize
new84.46 KB

Just realised I named the breadcrumb.css incorrectly

lewisnyman’s picture

StatusFileSize
new1.96 KB
new84.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

Status: Needs work » Needs review
StatusFileSize
new2.11 KB
new84.44 KB

Fixing tests

sqndr’s picture

Issue summary: View changes

Updated the summary.

sqndr’s picture

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

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

Category: Task » Bug report
Status: Fixed » Active
StatusFileSize
new90.22 KB
new90.88 KB

Sorry it added a CSS regression.

D8 now

D8 before

lewisnyman’s picture

StatusFileSize
new319.58 KB

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

jibran’s picture

it is fine with aggregate but not without it.

Aggregate on

Aggregate off

lewisnyman’s picture

StatusFileSize
new504.27 KB

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

Here's a shot from Simplytest.me

jibran’s picture

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.