Split Bartik's CSS into SMACSS style components

Let's not fix any problems we fix while moving files.

Is this a typo? Does it mean "not fix any problems we see..." ?

I just want to reiterate for anyone new, the CSS which handles colors that can be changed by Color module need to be in a separate style sheet — the way it's structured in Bartik for D7. This will be a deviation from the core OCSS plan, but it's needed to prevent much head banging. I went into much more detail about why here: https://www.drupal.org/node/2372045#comment-9338781

Putting to Needs review to see failing tests

Worked on this issue on the DrupalCamp Gothenburg Sprint Day. (Re)Created files, directories, and moved files and content according to https://www.drupal.org/files/issues/split_bartik_s_css_into-2375673-4.patch up until line 496 (breadcrumb.css created, Breadcrumbs css content added).

Found some issues, e.g. with a missing book.css file that was not added to bartik.libraries.yml file, which I fixed.

Pls feel free to pick this up and continue work on it.

Drupal 8 commit version was:

commit 1397bd65bc2614287621622b113396593333813c
Author: Nathaniel Catchpole

Color module can rewrite an array of stylesheets, so that could be SMACCified also if we wanted to go that far.

Working on this today 22.11.14 on finnish Drupal user groups sprint. Assigned to myself.

Did a manual reroll for patch as some css had changed. It would be very good not to make the changes before split is done. I have a backup of style.css before the change and I'm going to add it to next message tomorrow morning.

Didn't touch the tests. Just made a split for css's.

Putting to needs review to see what testbot has to say

Last patch lacked few things f.ex bartik.libraries.yml file. Also added few tests.

Tried to fix the tests.

While looking into split with fresh pair of eyes around we made a notice that even stuff from layout.css was moved to layout/layout.css the ole layout.css was still around. We also found an failure from CssCollectionGrouperUnitTest as elements.css had wrong basename.

I didn't touch the file print.css -file as I think it should be handled separately. Another completely wrongly being thing is the components/media.css but also for that I think that best way to handle it is after the smaccsifying has been done and separately to this issue.

Fixed the taxonomy and menu tests. Should be one failure left.

To get testbot stuff.

We need the beta evaluation block to the issue summary

We still have some visual changes on the layout

Before:

After:

Typo on buttons.css on bartik.libraries.yml. Fixed it.

Problem with the sidebars is that it color.css sets color for the borders but its being overrided on the sidebar.css with border: 1px solid;. We could use border-width: 1px; border-style: solid; in the sidebar.css to avoid this problem.

Fixed the broken border.

1. +++ b/core/themes/bartik/bartik.libraries.yml
   @@ -2,9 +2,45 @@ base:
   +      # Todo, remove media-queries to correct files
   +      css/components/media.css: {}
   ...
   +      # Todo remove print css
          css/print.css: { media: print }
   

   I don't think we should have todo's in this file, let's just create follow up issues. Also should we be removing the print css? Is there an issue for that?

2. +++ b/core/themes/bartik/bartik.libraries.yml
   @@ -2,9 +2,45 @@ base:
   +      # Theme
   +      css/theme/maintenance-page.css: {}
   

   We are also loading this css file unnecessarily, as it is being loaded in it's own library below. We should update the page for that css file

1. +++ b/core/themes/bartik/css/components/breadcrumb.css
   @@ -0,0 +1,5 @@
   +.breadcrumb {
   +  font-size: 0.929em;
   +}
   \ No newline at end of file
   
   +++ b/core/themes/bartik/css/components/captions.css
   @@ -0,0 +1,31 @@
   +[dir="rtl"] .caption-blockquote > figcaption {
   +  text-align: right;
   +}
   \ No newline at end of file
   
   +++ b/core/themes/bartik/css/components/featured.css
   @@ -0,0 +1,22 @@
   +#featured p {
   +  margin: 0;
   +  padding: 0;
   +}
   \ No newline at end of file
   
   +++ b/core/themes/bartik/css/components/highlighted.css
   @@ -0,0 +1,6 @@
   +#highlighted {
   +  border-bottom: 1px solid #d3d7d9;
   +  font-size: 120%;
   +}
   \ No newline at end of file
   
   +++ b/core/themes/bartik/css/components/messages.css
   @@ -0,0 +1,17 @@
   +[dir="rtl"] div.messages {
   +  margin-right: 23px;
   +  margin-left: 15px;
   +}
   \ No newline at end of file
   
   +++ b/core/themes/bartik/css/components/shortcut.css
   @@ -0,0 +1,15 @@
   +div.add-or-remove-shortcuts {
   +  padding-top: 0.9em;
   +}
   \ No newline at end of file
   

   Missing new lines at the end of files

2. +++ b/core/themes/bartik/css/components/content.css
   @@ -0,0 +1,177 @@
   +/* ----------------- Content ------------------ */
   +
   

   Content seems like a very generic name for a file. It looks like a lot of the classes in here are to do with nodes. So node.css might be a good idea.

   There are also a lot of field CSS so maybe we should move them to field.css?

   node-preview should go into it's own CSS file that that is it's own component.

3. +++ b/core/themes/bartik/css/components/header.css
   @@ -0,0 +1,222 @@
   +/* ------------------ Header ------------------ */
   +.skip-link,
   +.skip-link.visually-hidden.focusable {
   

   Skip link should go into it's own file.

4. +++ b/core/themes/bartik/css/components/list.css
   @@ -0,0 +1,67 @@
   +.pager .pager__items {
   +  padding: 0;
   +}
   +.pager__item {
   +  font-size: 0.929em;
   +  padding: 10px 15px;
   +}
   

   pager styles should go into their own CSS file

5. +++ b/core/themes/bartik/css/components/list.css
   @@ -0,0 +1,67 @@
   +ul.tips {
   +  padding: 0 0 0 1.25em; /* LTR */
   +}
   

   tips look like it's own component as well

6. +++ b/core/themes/bartik/css/components/main.css
   @@ -0,0 +1,6 @@
   +/* ------------------- Main ------------------- */
   +
   +#main {
   +  margin-top: 20px;
   +  margin-bottom: 40px;
   +}
   

   Maybe this can just go into layout.css?

7. +++ b/core/themes/bartik/css/components/search.css
   @@ -0,0 +1,71 @@
   +/* --------------- Search Results ---------------- */
   +ol.search-results {
   +  padding-left: 0;
   +  list-style-position: inside;
   +}
   

   Can we split up search form and search results into separate files? They are separate components

8. +++ b/core/themes/bartik/css/components/tabs.css
   @@ -0,0 +1,157 @@
   +/* ------------------ Reset Styles ------------------ */
   +
   +blockquote {
   

   Looks like this styling should go into base/elements.css?

9. +++ b/core/themes/bartik/css/components/views.css
   --- /dev/null
   +++ b/core/themes/bartik/css/hacks.css
   
   +++ b/core/themes/bartik/css/hacks.css
   +++ b/core/themes/bartik/css/hacks.css
   @@ -0,0 +1,15 @@
   
   @@ -0,0 +1,15 @@
   +/* -------------- Other Overrides ------------- */
   +
   +div.password-suggestions {
   +  border: 0;
   +}
   

   Can we not have a hacks file please? Can we move these styles into meaningful file name? Maybe user.css/forum.css/contextual.css?

Thanks for working on this issue @DickJohnson! To make reviewing easier next time you should create interdiff.

Fixed the problems on coding standards. Fixed bartik.libraries.yml for all parts reported. Got rid of main.css, made a set of new files to components. Thanks for tip on interdiff, going to create that kind of thing next time I'll make a patch.

I left few unwanted files like hacks.css and media.css around as those are mostly the kind of things we should get rid of when doing more refactoring & recoding of bartik. On this issue the importantest thing was the split. Still, if you want to name hacks.css as common.css or contextual.css or whatever we can do it.

Added interdiff for last two patches.

I've been trying to figured out why the patch is failing. I've narrowed it down to some change in the libraries file. If I delete the last 10 css files from the libraries.yml file then the test seems to pass. I have no idea why. See the interdiff

After talking with @lewisnyman we decided to try different kind of setup on bartik.libraries.yml to see what happens.

@sqndr: The problem we had there was really tricky. When I looked it I couldn't find any connection between the failing test and this patch. Both ways CSS files were loaded and visually everything was good. Any how this fixes the test.

Libraries were not attached on the info file so they were not loaded anywhere. I added them to the info file and I see something like this so this still needs work:

Putting still to needs review for testbot

Ok, so my first test went well because the css-files weren't actually added because of lackness of css/components on bartik.info.yml. Now the issue is being "fixed" (read: not getting failure from testbot) if you drop any 6 css files from components on bartik.libraries.yml. So it looks like there somekind of limit for 26 files on one tree in yml file. Makes no sense.

Assigning to Wim, that is his specialty :)

And as a proof a patch and interdiff.

Wow.

I haven't finished debugging yet, but I'm betting it has something to do with the different way of loading CSS files to support IE's silly limitations. From CssCollectionRenderer:

        // For file items, there are three possibilities.
        // - There are up to 31 CSS assets on the page (some of which may be
        //   aggregated). In this case, output a LINK tag for file CSS assets.
        // - There are more than 31 CSS assets on the page, yet we must stay
        //   below IE<10's limit of 31 total CSS inclusion tags, we handle this
        //   in two ways:
        //    - file CSS assets that are not eligible for aggregation (their
        //      'preprocess' flag has been set to FALSE): in this case, output a
        //      LINK tag.
        //    - file CSS assets that can be aggregated (and possibly have been):
        //      in this case, figure out which subsequent file CSS assets share
        //      the same key properties ('group', 'every_page', 'media' and
        //      'browsers') and output this group into as few STYLE tags as
        //      possible (a STYLE tag may contain only 31 @import statements).

My suspicion was correct. But not in the way I thought it'd be; it's a very subtle, very ridiculous problem! :)

It's a combination of:

1. a not specific enough test: using assertText() to verify strings are present that happen to also be CSS file names (system.theme is a config entity and part of the name of the CSS file /core/modules/system/css/system.theme.css
2. a silly, silly subtle difference in how assertText() works on <link rel="stylesheet" href="…/system.theme.css"> (HEAD, because there are much fewer CSS files to load) versus <style>@import("…/system.theme.css")</style> (with patch, because we exceed the 31-file limit cited in #62): in the former, the CSS file is in an attribute of the element and hence doesn't match, in the latter, the CSS file is in the textNode of the element and hence does match. I think it'd be fair to say that we never ever ever want to match the text content of a <style> tag, but that's unfortunately how it works in HEAD.

Attached is the suggested interdiff to fix ConfigImportUITest, which makes it more specific. I'll leave it to the people active in this issue to choose which patch to apply this suggested interdiff to; it's not quite clear to me which it should be applied to :)

Rerolled to #45 and applied Wim Leers patch.

+++ b/core/modules/config/src/Tests/ConfigImportUITest.php
@@ -180,16 +181,16 @@ function testImport() {
+ ¶

Starts looking better. Could we remove that extra space?

Sure.

Menu is still broken:

Yep, this is why I was saying in IRC that the CSS was still broken; during the splitting up into multiple files, some specifically ordered CSS was probably changed.

I guess we shouldn't place them in alphabetical order for now until we have cleaned up to selectors so conflict less.

Yeah. Media.css must be loaded after others and also files were pointing to wrong directories.

Accidentally removed bartik.libraries.yml from the latest patch.

Tried to reroll.

Fixed the issues on ConfigImportUITest.php.

To see testbot.

Reroll. I can't see why this would fail now. Where are we changing Bartik settings in this test?

This patch is not gonna apply because it needs reroll. This should fix the tests.

RErolled

Last patch didnt remove the styles.css file

I'm afraid this looks very broken:

Probably a silly mistake? :)

I will take a look at this. It's also important to verify that we aren't loosing CSS changes that have been committed when we reroll

I used this command to find all CSS changes in Bartik since: #22

git log --since="2014-11-22" core/themes/bartik/css/

The output was:

commit e89029ce18103ee963c390d526f6d66d6c5f49f6
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Mon Dec 1 13:26:32 2014 +0000

    Issue #2368251 by emma.maria, vermario, tkoleary: No border around image upload widget when creating content in Bartik

commit 4fef7db1d97d895be4a26c87016c417d6b76460d
Author: Dries <dries@buytaert.net>
Date:   Wed Nov 26 16:53:27 2014 -0500

    Issue #2365653 by emma.maria, stefan.korn: CSS definition for one sidebar and 560 to 850 px not correct

The paths for colors.css and layout.css were wrong. I also changed global-styling to global_styling because it seems consistent with maintenance_page?

Also, we did regress on #2365653: CSS definition for one sidebar and 560 to 850 px not correct so i fixed that.

#92: I'd say: stick to global-styling at least for now, so that it's consistent with every other theme in core? But it's a tiny, tiny detail, so you can choose what you want. We have periods, dashes and underscores all across core. That's another thing to standardize… :P
(Also: AFAICT we use dashes to separate words in filenames as well, including in Bartik, so we probably want to apply the same logic here?)

Other than that: this one works! It looks exactly the same. What's the best way to get this to RTBC? Sit down with two D8 sites next to each other, look at many pages, try to spot any differences?

I'd say: stick to global-styling at least for now

Sure thing.

It looks exactly the same. What's the best way to get this to RTBC? Sit down with two D8 sites next to each other, look at many pages, try to spot any differences?

Let's make a list of test pages and put them in the issue summary. I might get a PhantomCSS install running to check the pages

I might get a PhantomCSS install running to check the pages

WOOT! :)

Added some initial steps to the IS. Feel free to expand.

I spent few hours on testing this. Testing was done with latest Chrome and Firefox.

What I did:
1. Added different kind of content of various content types. Long nodes, short nodes, comments etc.
1.1 Also custom blocks
2. Created views listing content, and taxonomy terms inc descriptions
3. I placed the content, blocks and views to all regions, one by one
4. Checked all the listing pages, checked user page, checked article & basic page pages.
5. Added new menus to different regions
6. If I saw anything unusual I took a clean install of Drupal 8 and compared

What I noticed:
There was a lot of moments where I thought something is broken but it wasn't. From my point of view everything is working in the patch.

And as a proof a screenshot that I actually did something. There's no sane way to screenshot that everything is working.

So I'd say RTBC.

-Erno

Ok, I played around with BackstopJS and found it to be very useful indeed. I've attached my configuration file, renamed to backstop.json._txt, so instead of running gulp genConfig you can just copy my config into the correct place and make sure that it's rename to the .json file extension. You will have to change the URLs to your local install though.

I found some regressions! Some of them are very subtle. The only problem is that the report runs locally and I haven't found a way to share it in a .zip or flat html. The best I can do now is take screenshots of the failures.

There seems to be two kind of issues: regression and non-regression issues. Example on non-regression issue is on menu right corner. The bug really is because of ../../../ instead of ../../../../

Here is a patch fixing the non-regression issue.

It looks like basic copy-paste error instead of regression after all.

Forgot the patch.

The latest patch still has some visual failures.

The sidebar has moved higher up at every screen width...
For example on desktop when it sits on the left of the main content it is higher up.

Desktop testing:

Before patch:

After patch:

And on mobile the same thing happens the sidebar is underneath the main content section, it sits higher up too...

Mobile testing:

Also the primary tabs lose their horizontal line across at every screen width...

Seen in...

Also when Bartik is the admin theme, all of the above happens, plus dropbuttons have lost their styling completely....

Desktop testing...

Mobile testing...

Clear screenshot...

Also with the file structure can we please try to remove hacks.css. If it's contents cannot be placed in a CSS file, please rename to _overrides.css or something similiar.

From all the above this needs a little more work. But it's getting there :)

Edit: I have highlighted the bugs in bold to help them to stand out.

The tabs issue was a relative image url.
The sidebar issue was a CSS ordering issue, fixed by moving layout.css back up to the top.
The dropbutton problem was because CSS files are still set to replace each other if they have the same name, and for some reason the misc/dropbutton.css is being loaded after the theme dropbutton.css and overwriting it. This is annoying but not a problem to fix in this issue. I copied the naming from Seven and called the file dropbutton.component.css.

Looks like we're almost there?

I did huge amount of testing on this and it seems to be working mostly. I've tested all the core modules, even the ones disabled by default. Only thing I could find was these tabs are still having some regression:

Nice, I fixed this problem by moving the vertical tabs CSS from form.css into another file. The filename also has to include .component because there is already a vertical-tabs.css in core. This is still really annoying so I've created an issue here: #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name

Good job! Looks good to me. I can say visually this is RTBC. I haven't looked at the code so I won't RTBC this.

IMHO this makes it much easier to work on Bartik's CSS. Much easier to navigate around. Hurray :)

Here's a code review. It looks great, so only nits.

1. +++ b/core/themes/bartik/bartik.libraries.yml
   @@ -2,8 +2,44 @@ global-styling:
   +      css/components/dropbutton.component.css: {}
   ...
   +      css/components/vertical-tabs.component.css: {}
   

   Perhaps comments for explaining the reason for the inconsistent filenames would be useful?

2. +++ b/core/themes/bartik/css/components/buttons.css
   @@ -0,0 +1,25 @@
   +/* ---------------- Buttons    ---------------- */
   +
   +.button {
   
   +++ b/core/themes/bartik/css/components/captions.css
   @@ -0,0 +1,31 @@
   +/* -------------- Captions -------------- */
   +.caption {
   
   +++ b/core/themes/bartik/css/components/content.css
   index 0000000..0b08395
   --- /dev/null
   
   --- /dev/null
   +++ b/core/themes/bartik/css/components/contextual.css
   
   +++ b/core/themes/bartik/css/components/contextual.css
   +++ b/core/themes/bartik/css/components/contextual.css
   @@ -0,0 +1,4 @@
   
   @@ -0,0 +1,4 @@
   +#header .contextual .trigger,
   

   Are these headers still necessary? Aren't they implied by the filename?

   Also: sometimes, they have a newline after them, sometimes they don't. And occasionally, there even is no header at all.

3. +++ b/core/themes/bartik/css/components/forum.css
   @@ -0,0 +1,6 @@
   \ No newline at end of file
   
   +++ b/core/themes/bartik/css/components/vertical-tabs.component.css
   @@ -0,0 +1,5 @@
   \ No newline at end of file
   

   Missing trailing newline.

@Wim Leers for #112

1. We had to name the files like this as for some reason the system CSS files of the same names were stopping the Bartik ones from loading and we were missing CSS. We have this file naming structure in Seven too.

2. We are fixing the headers in a follow up patch or patches. We wanted to just get everything split up, leave the code as it is and then we can improve each stylesheet separately in issues instead of it all being in one monster patch.

3. But yes we do need to fix trailing spaces however! They are a result of the work in this issue.

Setting to Needs Work to fix 3.

#113:

1. I know :) I'm only saying that it'd be great to add a comment to the YML file to document that, because otherwise surely people will start to open issues for making it consistent.
2. Ok, sounds fair :)
3. Please also do 1, so I don't feel super silly for marking this NW for only trailing newlines :P

Hate to say this but after changes to core today, the patch no longer applies.

Checking patch core/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.php...
Hunk #1 succeeded at 91 (offset -20 lines).
error: while searching for:
    $this->assertSame($groups[5]['media'], 'all');
    $this->assertSame($groups[5]['preprocess'], TRUE);
    $this->assertSame(count($groups[5]['items']), 1);
    $this->assertContains($css_assets['style.css'], $groups[5]['items']);

    // Check group 7.
    $this->assertSame($groups[6]['group'], 100);

error: patch failed: core/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.php:193
error: core/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.php: patch does not apply

This patch fixed Wim Leers' points 1 & 3 from #112. Btw last patch wasn't removing the styles.css so ignore it completely.

#118 is identical code-wise to #110 (I manually compared), it was merely a rebase on latest HEAD. It was RTBC visually (style-wise) as of #111. It only needed a code review. I just did that.

So: RTBC! :)

I tested the newest patch visually and the issues in #106 have been fixed.
I also applied the patch and it applied cleanly with no whitespace errors.

RTBC++

+++ b/core/modules/config/src/Tests/ConfigImportUITest.php
@@ -102,20 +102,20 @@ function testImport() {
-    $this->assertText($name);
-    $this->assertText($dynamic_name);
-    $this->assertText('core.extension');
-    $this->assertText('system.theme');
-    $this->assertText('action.settings');
+    $this->assertRaw('<td>' . $name);
+    $this->assertRaw('<td>' . $dynamic_name);
+    $this->assertRaw('<td>core.extension');
+    $this->assertRaw('<td>system.theme');
+    $this->assertRaw('<td>action.settings');
...
-    $this->assertNoText($name);
-    $this->assertNoText($dynamic_name);
-    $this->assertNoText('core.extension');
-    $this->assertNoText('system.theme');
-    $this->assertNoText('action.settings');
+    $this->assertNoRaw('<td>' . $name);
+    $this->assertNoRaw('<td>' . $dynamic_name);
+    $this->assertNoRaw('<td>core.extension');
+    $this->assertNoRaw('<td>system.theme');
+    $this->assertNoRaw('<td>action.settings');

@@ -177,15 +177,15 @@ function testImport() {
-    $this->assertText('core.extension');
-    $this->assertText('system.theme');
-    $this->assertText('action.settings');
+    $this->assertRaw('<td>core.extension');
+    $this->assertRaw('<td>system.theme');
+    $this->assertRaw('<td>action.settings');
...
-    $this->assertNoText('core.extension');
-    $this->assertNoText('system.theme');
-    $this->assertNoText('action.settings');
+    $this->assertNoRaw('<td>core.extension');
+    $this->assertNoRaw('<td>system.theme');
+    $this->assertNoRaw('<td>action.settings');

Why all these changes? I don't see how they are related?

@Tim: see #62 and #64. Basically: those tests verify config keys showing up in the HTML source, but some of those keys happen to be identical to asset file names!

I'm sorry but I found one more bug on the RTL layout:

@sqndr: that's the node form in a narrow viewport.

I tested this scenario without the patch and it already exists in HEAD. This problem was not introduced in this patch.

I can confirm lewisnymans report. It exists in core already.

Phew, it was a ghost fail :D

The pre-existing RTL problems in #125–#128 are going to be fixed by #2329649: Fix node create page RTL CSS and don't conflict with this patch :)

Patch doesn't apply to latest head. Needs reroll.

No, sorry, it should work after all.

Wow, great work on this, folks! Exciting to see Bartik getting some love in D8. :)

I will fully admit I did not review every line of this patch. :) However, I can see that lots of effort and testing went into this, and if it does introduce some bugs, much easier to fix them in tiny ~500b patches than as part of a ~150K patch.

Since this is a markup-affecting patch, it's under the list of things we can commit at https://www.drupal.org/contribute/core/beta-changes, committed and pushed to 8.0.x. YEAH! :)

Yay an early Christmas present! Thanks Webchick, thanks all for your hard work.