Clean up system admin css

https://gist.github.com/3006391#L3676

I've ignored ID selector warnings since these are admin screens and we should be safe there.
I've also ignored heading warnings since I don think they apply to our situation as a CMS.

Needs more work.

/admin/reports/status looks bad after applying the patch.
Before: http://cl.ly/image/46163L2q3I2Z
After: http://cl.ly/image/2M070b1P2e1v

This is due to the fact that the transparency got removed:

.system-status-report td:nth-child(-n+2) {   
  background-color: rgb(0, 0, 0);
}

Should be: background-color: rgba(0, 0, 0, 0.04);

+++ b/core/modules/system/css/system.admin.css
@@ -187,39 +187,40 @@ a.module-link-configure {
-  background-color: rgba(0, 0, 0, 0.04);
...
+  background-color: rgb(0, 0, 0);

This should not be changed. See comment #7.

Attached is a new patch that changes that line.

8: 1663166-8-cleanup-system-admin-css.patch queued for re-testing.

Ah, cool. I remember you talking about that tool. Thanks for sharing it. ;)

Patch is green, before/after pages have been tested. Let's go.

Hey, sorry. I found a few more areas that we could clean up:

Removed all colours declared in system.admin.css

The system module should not have an opinion about color. Lets leave that to the themes.

#system-modules label {
  color: #1d1d1d;
  font-size: 1.15em;
}

Remove units from 0 values
Lets replace '0px' with '0'

#system-modules td details {
  border: 0px;
  margin: 0px;
  height: 20px;
}

1. +++ b/core/modules/system/css/system.admin.css
   @@ -6,40 +6,40 @@
   -div.admin {
   +.admin {
   

   Recipe for false-positives.

   .admin is too short and too common/ambiguous to stand on its own.

2. +++ b/core/modules/system/css/system.admin.css
   @@ -83,11 +83,9 @@ small .admin-link:after {
    #system-modules label {
   -  color: #1d1d1d;
   
   @@ -105,22 +103,21 @@ small .admin-link:after {
    #system-modules td details a {
   -  color: #5C5C5B;
   
   @@ -144,16 +141,9 @@ small .admin-link:after {
   -span.admin-enabled {
   -  color: #080;
   

   Same as my note about borders - these changes require before/after screenshots of Stark.

3. +++ b/core/modules/system/css/system.admin.css
   @@ -251,7 +239,7 @@ table.system-status-report .status-title {
    .system-themes-list-disabled {
   -  border-top: 1px solid #cdcdcd;
   +  border-top: 1px solid;
   
   @@ -270,7 +258,7 @@ table.system-status-report .status-title {
    .theme-default .screenshot {
   -  border: 1px solid #aaa;
   +  border: 1px solid;
   

   Why are we removing / defaulting the border color to black everywhere?

   Shouldn't we default to some basic/minimalistic colors instead? (potentially using built-in color shortnames; black/grey/silver/etc)

   Alternatively, we could consider to drop the borders altogether. (Doing so would require a fair amount of testing + screenshots though.)

   I'd personally prefer to use simplistic color shortnames (so as to produce a minimalistic but yet working output).

Recipe for false-positives.

.admin is too short and too common/ambiguous to stand on its own.

Good point! Let's fix this here.

Why are we removing / defaulting the border color to black everywhere?

Shouldn't we default to some basic/minimalistic colors instead? (potentially using built-in color shortnames; black/grey/silver/etc)

In my mind, the less CSS we write on top of browser defaults the better because then we have less that other themes have to overwrite. I don't think Stark has to "look good" but I don't know if you agree with that?

So, after #2017257: Create generic layout classes it looks like the admin class is no longer used in system.admin.css, let's just remove the class from the mark up and bid good riddance.

I've reimplemented the patch, completely removing the admin class and not touching the colours.

The admin class isn't being used anymore.
Removing colours without adding them back in Seven is going to cause visual regressions so we should do that elsewhere.

We could do with some before/after screenshots just to make sure we haven't broken anything.

There is something wrong, the patch failed:

Checking patch core/modules/system/css/system.admin.css...
error: while searching for:
/**
 * Modules page.
 */
#system-modules thead > tr {
  border: 0;
}
...
error: patch failed: core/modules/system/css/system.admin.css:47

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -14,7 +14,7 @@
-<div class="admin clearfix">
+<div class="clearfix">

I'm strongly suggest that don't leave any single layer with .clearfix only. It's less semantic meaning & not so BEM way.

Reroll.
Needs work(decision?) to do something with the #26 comment.

Status change.

@droplet What would you suggest we do with this clearfix?

Fixing tags.

@droplet: Any ideas on this? Removing this might break everything?

I discussed it with Lewis and we couldn't think of a reason why we would need the wrapper anyway. So I removed the whole .admin wrapper div, and also took out the unneeded clearfix classes in the columns.

Before:

<?php
<div class="admin clearfix">
  {{ system_compact_link }}
  {% for container in containers %}
    <div class="layout-column half clearfix">
      {% for block in container.blocks %}
        {{ block }}
      {% endfor %}
    </div>
  {% endfor %}
</div>
?>

After:

<?php
{{ system_compact_link }}
{% for container in containers %}
  <div class="layout-column half">
    {% for block in container.blocks %}
      {{ block }}
    {% endfor %}
  </div>
{% endfor %}
?>

Looks good.

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -14,13 +14,11 @@
+{{ system_compact_link }}
+{% for container in containers %}
+  <div class="layout-column half">
+    {% for block in container.blocks %}
+      {{ block }}
+    {% endfor %}
+  </div>
+{% endfor %}
\ No newline at end of file

There is no new line on the end of this file. I can't see any other problems.

Good catch!

I manually tested all the affected pages and they look great. Here are screenshots in Seven and Stark. The extend page looks bad on Stark in HEAD so it is not introduced in this patch.

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -14,13 +14,11 @@
-<div class="admin clearfix">

We use this div to style...

/* Configuration. */
div.admin .right,
div.admin .left {
  width: 49%;
  margin: 0;
}

from bartik.

@media screen and (min-width: 40em) {
  div.admin .right,
  div.admin .left {
    width: 49%;
    margin: 0;
  }
}

from seven

Good catch. This piece of CSS doesn't seem to be used anymore. I tested both Seven and Bartik and they work fine without the wrapper div and these CSS rules. They are divided into columns by this piece of CSS:

.layout-column.half {
  width: 50%;
}

EDIT: That interdiff should have been a .txt file :(

@alexpott Good catch! @BarisW Thanks for the patch.

We look good to go now. Annoying that we missed this duplicate code when we implemented the layout classes.

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -14,13 +14,11 @@
-<div class="admin clearfix">

So we've covered the admin part of this... but what about the clearfix?

Baris and I discussed this in Ghent but I guess we forgot to mention it in the issue. The clearfix class has no effect on the page, as there is no content below the floated content.

The clearfix is important.
Steps to reproduce:

1. Add any old block to seven's content region beneath the main content block.
2. Go to admin/config
3. See visual regression

Ah I see. Good point. In that case we can try replacing the clearfix class with the .layout-container class. This includes a clearfix.

@LewisNyman here's how that looks like. Layout is fixed, but a new margin on both sides in introduced with the class (margin: 0 2.5em;).

Here is how it looks without any patch applied atm for comparsion:

Ah yep that doesn't work :-( I guess we should just go back to good old clearfix?

OK, back to clearfix...

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -14,10 +14,10 @@
+<div class="clearfix">

can we add ".layout-container .clearfix".

@droplet: no, because that introduces left and right margin. See comment #47.
Patch in #50 is RTBC according to me.

RTBC from me as well. Using a clearfix in this way in inline with our CSS standards. We don't need to add 'semantic' classes for styling hooks, just classes that have design meaning.

Can I say No ? hehe.

OK. still thinking of when you really want the system looks like #47 or whatever crazy idea. Applying margin to ".layout-whatever-drupal-code-standard-name" always better than ".block .clearfix".

#47 is a simple problem.

Change .layout-container to .layout-col-container or whatever fits Drupal Code Standard.
Or apply .block .layout-container { margin: 0 }

It's a great idea male. Because you soon can apply this pattern to everywhere:

.layout-column-container
--.layout-column
--.layout-column

Thanks.

Right I can see what you mean droplet. I think that such change is out of the scope of this issue though.

If it's out of the scope, I can't understand why it's also changing / removing other classes name.

OK @Manuel Garcia. I'll keep silence :).

@droplet I understand your suggestion, but the idea here is to minimise the default HTML/CSS as much as possible. We don't need to provide classes that aren't used because if someone wants to add some styling to this page then they can override the template file. No one should ever have to write .block .clearfix

This issue is an unfrozen change (CSS and templates) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 21765ac and pushed to 8.0.x. Thanks!