Clean up "layout" CSS in Bartik

I've just moved stuff from layout.css to appropriate component css file, didn't look about coding standard in this patch.

Fixed new line errors from previous patch...

Removed white space in page.css.

This patch should also make the switch from id selectors to classes in line with the Drupal CSS coding standards. This change was originally one issue, but was split into subtasks to make the transition easier. More info available #2347751: Remove id selectors from templates of Bartik

1. +++ b/core/themes/bartik/css/components/messages.css
   @@ -15,3 +21,11 @@ div.messages {
   diff --git a/core/themes/bartik/css/components/page.css b/core/themes/bartik/css/components/page.css
   

   I think we should keep the rest of the file in layout.css and shold not move it to a "page" component. (Page is not a compoent IMHO.)

2. +++ b/core/themes/bartik/css/components/sidebar.css
   @@ -29,3 +29,93 @@
   +  .layout-two-sidebars #content {
   

   The following rules are about #content so they should be in the content component.
   I was also thinking about if we should use modifiers like .content--one-sidebar, .content--two-sidebars, .content--first-sidebar etc. instead of using context.

This patch should also make the switch from id selectors to classes in line with the Drupal CSS coding standards. This change was originally one issue, but was split into subtasks to make the transition easier.

The remaining css in layout.css no longer has any id selectors. To complete the transition, the id selector would have to replaced in every file that references this selector. Instead, I added an additional class on the element with the id and used that selector instead. After each file has been cleaned, the id attribute can be removed from the template file.

I think we should keep the rest of the file in layout.css and shold not move it to a "page" component. (Page is not a compoent IMHO.)

Done

The following rules are about #content so they should be in the content component.
I was also thinking about if we should use modifiers like .content--one-sidebar, .content--two-sidebars, .content--first-sidebar etc. instead of using context.

Ideally, yes. But this would require preprocessing for each separate element instead adding a body class. The current implementation is a lot more practical, so I kept it for now.

+++ b/core/themes/bartik/css/components/sidebar.css
@@ -1,7 +1,76 @@
+@media all and (min-width: 851px) {
+  #content {
+    float: left; /* LTR */
+    position: relative;
+  }
+  [dir="rtl"] #content {
+    float: right;
+  }
+  .layout-two-sidebars #content {
+    margin-left: 25%;
+    margin-right: 25%;
+    width: 50%;
+  }
+  .layout-one-sidebar #content {
+    width: 75%;
+  }
+  .layout-no-sidebars #content {
+    width: 100%;
+  }
…

Should use the newly added .main__content class instead?

Also should be moved to components/content.scss

Keeping the body class usage is OK, I think. :)

Should use the newly added .main__content class instead?

Yes, correct. I updated the patch accordingly.

Also should be moved to components/content.scss

Done

Keeping the body class usage is OK, I think. :)

Alright :)

A small thing: maybe it would be better to use .main-content instead of .main__content because

• .main is not a component but a layout element (if I'm right)
• sidebars can be found within .main but we do not call them eg. .main__sidebar-first (and we should not)
• "content" itself is a component, just like a "sidebar"

If I'm right, "content.css" should be renamed to "main-content.css" too.

A small thing: maybe it would be better to use .main-content instead of .main__content because

• .main is not a component but a layout element (if I'm right)
• sidebars can be found within .main but we do not call them eg. .main__sidebar-first (and we should not)
• "content" itself is a component, just like a "sidebar"

Yes, either .main or .main-content would work for me. Bartik currently has a weird construction where a <div id="main"> wraps the <main> element. I have removed the redundant wrapper and added the .main-content class to the <main> element.

If I'm right, "content.css" should be renamed to "main-content.css" too.

Lets leave this for #2398463: Clean up the "content" component in Bartik depending on what gets committed for this issue.

Yes, either .main or .main-content would work for me. Bartik currently has a weird construction where a

wraps the element. I have removed the redundant wrapper and added the .main-content class to the element.

Totally agree but I think it's easier to keep these issues to just copying and pasting stuff out and into the correct files, and then using the other files to do the clean up and renaming. Otherwise it becomes really hard to tell if something has dropped out by mistake.

Anything left in layout.css should definitely be prefixed with .layout-

@LevisNyman Ohh. So no changes in moved code, only in the remaining code. I understand but sad because of made work idebr more than it was needed… :(

I add the related issues here to help who work on those keep an eye on this issue.

Hm, I'm not able to save related issues…

Ohh, I had to (start to) write the title of the issue instead of its id… :)

Just my two cents about the naming. Content.css will be renamed and splitted up anyways, so there's no need to handle it here.

Let's keep this issue focused. Maybe we should start again from the patch in #2398451: Clean up "layout" CSS in Bartik? It's very hard to keep track of changes if we are also moving the code around into different files. We have an issue to handle the naming conventions for content.css etc

@LewisNyman which patch? :)
You linked to this issue, but we have multiple patches. Guess you wanted to link specific comment?

@Dragan Eror Ah yes thanks, I meant the patch in #10

The patches after #10 only change id selectors to class selectors, update existing code to adhere to the current CSS coding standards and provides a base for other cleanup issues to work with. Taking these changes out to make reviewing a little easier seems like a step backwards to me.

Totally agree but I think it's easier to keep these issues to just copying and pasting stuff out and into the correct files, and then using the other files to do the clean up and renaming. Otherwise it becomes really hard to tell if something has dropped out by mistake.

The cleanup in files other than layout.css is relatively minor compared to the changes to the layout.css itself. The nice thing about these changes is all the selectors can be tested on a single (home-)page. Perhaps a report like #2399709: Remove media.css from Bartik, add it's current code directly to the components it references. would help or some full page screenshots would help?

Another thing: shouldn't we postpone #2398463: Clean up the "content" component in Bartik and #2398471: Clean up the "footer" component in Bartik until this issue is resolved? They are affected by the output of this issue.

Don't think so. Even there will be rerolls as we touch the same code etc the major discussion we have on content or footer clearup is mostly completely different.

Anything left in layout.css should definitely be prefixed with .layout-

Done

Screenshot desktop (before/after):

Screenshot desktop RTL (before/after):

+++ b/core/themes/bartik/css/layout.css
@@ -6,13 +6,13 @@
 /**
  * Section
  */
-.section {
+.layout-section {
...
-  .section {
+  .layout-section {

Sorry to be annoying, but I think we have an opportunity here to rename this class to something reusable and meaningful. This looks similar in purpose to th 'layout-container' class we use in the Seven theme. I think we can rename it to that?

Here is the patch rerolled from #2398451-28: Clean up "layout" CSS in Bartik. I've just simply replaced all "layout-section" to "layout-container".

Thanks, I don't think we need more screenshots.

+++ /Users/thamas/Projects/d8/2398451-30.patch  Sun Feb  1 09:34:41 2015
@@ -277,7 +277,7 @@
 +/**
 + * Section
 + */

We need to update this CSS comment to match the new class name.

We need to update this CSS comment to match the new class name.

Now it says: Container

Ah no reroll needed :(

Rerolled the patch and applied two fixes:

1. -#footer-wrapper {
   +#site-footer__wrapper {
      font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
   

   This line was not updated when the footer regions were renamed.

2.  .layout-main-wrapper {
      min-height: 300px;
   +}
   +.layout-main {
      margin-top: 20px;
      margin-bottom: 40px;
    }

   I had to reintroduce this wrapper since min-height together with margin has a different size than before where these selectors were applied on two different elements.

Looks good.

Compared layouts in different widths after reroll.
Desktop before:

Desktop after:

Tablet before:

Tablet after:

Mobile before:

Mobile after:

Mobile footer before:

Mobile footer after:

Drupal\system\Tests\Routing\ExceptionHandlingTest->testBacktraceEscaping()

I doubt that's related

Back to RTBC per #35 after testbot hickup.

Hm, this no longer applies for me.

rerolled the patch in #34

I diffed the two patches using diff 2398451-34.patch 2398451-41.patch. There seems to be a big difference between the two patches, I've attched the output. I'm not sure if this means we are missing something or not, but it probably needs another manual test.

I updated my local branch from #34 and I can confirm the reroll in #41 missed quite a big hunk in layout.css. Attached patch is a new reroll from #34 with the changes from #2422975: Bartik footer has CSS regressions. merged in.

Some before/after screenshots for good measure:

@idebr Hey, I think you missed the patch?

Ugh.. thanks

@emma.maria Thanks for the extensive testing :). I can confirm the media queries for the .region-header were missing. I have re-added them in the attached patch.

Sure! I was hesitant to remove it since the proposed resolution was not to change the existing design, but as long as you're okay with it :)

Reroll after #2398469: Clean up the "Featured-top" component in Bartik was committed.

I compared the two patches and they seem to be very similar. So setting back to RTBC.

+++ b/core/themes/bartik/templates/page.html.twig
@@ -147,14 +147,14 @@
+          <div id="sidebar-first" class="column sidebar sidebar--first">
...
+          <div id="sidebar-second" class="column sidebar sidebar--second">

@@ -164,7 +164,7 @@
+        <aside id="featured-bottom" class="section layout-container clearfix" role="complementary">

Why are we introducing the .sidebar--first and .sidebar--second classes but not a .featured-bottom? This change seems to be unnecessary to complete the task of the issue and out-of-scope.

Fixed the issues mentioned by @alexpott in #57.

The id's were updated to classes since our CSS coding standards dictate id's should not be used to styling when a class can used instead. The patch doesn't touch any css from #featured-bottom, so the id is not converted. If this is out of scope, the updated patch in #58 addresses this feedback appropriately.

Reroll after #2398461: Clean up the "comments" component in Bartik was committed.

The reroll looks good!

The patch doesn't touch any css from #featured-bottom, so the id is not converted. If this is out of scope, the updated patch in #58 addresses this feedback appropriately.

That is correct, this issue is mainly about moving CSS into the correct component files so they can then be cleaned up.

https://www.drupal.org/node/1273052 has broken the last patch, with core/themes/bartik/css/layout.css not applying any more.

Merged in the change from #1273052: Footer container contents are wider than other page elements in Bartik.

Reroll looks good!

Reroll after #2289917: Convert "messages" page element into blocks was committed.

Reroll looks good!

Committed and pushed to 8.0.x. Thanks!