Clean up CSS and markup for status report

In an effort to get a better picture of issues remaining in the HTML5 Initiative, we're removing issues that are not directly HTML5-related. Note: Any issues assigned to the "Markup, CSS and JavaScript" components will still be broadcast on the HTML5 Twitter feed so that interested parties can participate. It might be more accurate to put this in the Markup component. I'll leave that up to you. :)

No reviews please :) I will reroll it after related issue got committed.

Also seems like I can downgrade this patch to D7 now: #1001932: Clean up Status report CSS (and IE fix)

myself expatiation and further suggestions :)

+++ b/core/modules/system/system.admin.cssundefined
@@ -97,30 +97,35 @@ a.module-link-configure {
-  background-color: rgba(0, 0, 0, 0.04);

remove bg color in stark theme

+++ b/core/modules/system/system.admin.cssundefined
@@ -97,30 +97,35 @@ a.module-link-configure {
+  border-bottom: none;

created an issue to see if it possible to get rid of border from global.
#1601196: remove TH border from base.css

+++ b/core/themes/seven/style.cssundefined
@@ -500,18 +500,18 @@ table tr.selected td {
+.system-status-report tr.ok {
...
+.system-status-report tr.warning {
...
+.system-status-report tr.error {

I preferred to remove TR. looking for your comments.

Also, there's one good thing I suggested in #1001932: Clean up Status report CSS (and IE fix) to optimize table rendering performance. (Maybe a new issue to apply it to all possible tables in Drupal)

+table.system-status-report{
+  table-layout: fixed;
+}

ref:
http://msdn.microsoft.com/en-us/library/ie/ms531161(v=vs.85).aspx

no ? or anything I'm missing too ?

We should use TH elements instead of TD.status-title.

It's using TH now

The icon can possibly be merged into the heading column by using some more fancy CSS (which ensures that a heading [which doesn't fit into the cell and wraps] continues below the heading instead of below the icon).

Usability issue, "CSS only" isn't the good way. But I merged it into one column

It's debatable whether Stark should contain the base styling that ensures a sane width for the heading column and also the background on the heading column. The switch to TH might make this obsolete (for Stark).

some css fixes.

Just adding a related issue.

correcting the tag to the more common one.

Icon is merged into the heading column, and the heading is continuing under heading instead of under the icon.

Here is the requested screenshot.

Removing tag. Adding image provided by @Aleksandar_P

This patch is very out of date. I think it's better to start again and see what we can improve. I've uploaded the CSS so it can be reviewed in Dreditor

1. .system-status-report td {
   ...
   .system-status-report .status-icon {
   ...
   [dir="rtl"] .system-status-report .status-icon {
   ...
   .system-status-report .status-icon div {
   ...
   .system-status-report .error .status-icon div {
   ...
   .system-status-report .warning .status-icon div {
   ...
   .system-status-report .status-title {
   

   We need to SMACSS up these classes, see: https://www.drupal.org/node/2408617

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

   I think we can remove this styling because it is overidden by Seven's table.css. Can we also remove the override in Seven here?

Looking into this, what of the old changes we can still implement and if the smacss'ing should be in a follow-up ticket for clarity purposes...

As expected, the last patch (#11) no longer applied, so I re-implemented all the needed changes following the new semantics (so no interdiff needed).

So merged the first 2 td's in one th as suggested, and adjusted the css accordingly. (What will we need for RTL?)

The smacss'ing up of the css has apparently been solved in another ticket? The only thing I should change is the div with class description, but I am not sure if this is something we use frequently?

Please review! (The use of th now causes a font-weight bold, but not really a fan of resetting this to a normal font weight because it looks better..)

This is looking good, but now we have a row of <th>'s and a column of <th>'s. Is that valid markup? I ran it through the W3C markup validator and it did complain a little:

A table row was 2 columns wide, which is less than the column count established by the first row (3).

1. +++ b/core/modules/system/css/system.admin.css
   @@ -188,24 +188,27 @@ small .admin-link:after {
   -.system-status-report td {
   +.system-status-report th {
   

   Can we create a class to apply this to the correct cells instead of the th elements?

2. +++ b/core/modules/system/css/system.admin.css
   @@ -188,24 +188,27 @@ small .admin-link:after {
   +  /*
   +  padding-right: 0;
   +  padding-left: 6px;
   +  */
   

   These properties are commented out

I'm at DrupalCon LA and I can update the patch to reinstate the commented out code

The .admin-link css wasn't added in this patch, so I'm guessing best to create a new issue for that?

Un-commented out the rtl css lines and checked the pages look good in LTR and RTL. Attaching new patch and interdiff

Adding screenshots

1 suggestion:
- Adding a class to right column (td)

Apparently, the status report-page is the only page that uses hidden headers as the only thead, so we should remove it's <thead>-tag. or not hide it.
I am keen on removing it because the table itself speaks for itself and the labels in the thead don't add anything for anyone in any situation I can think of.

@Lewis: the error the W3C-validator threw was because we had 3 table-elements inside a tr inside the thead and we had 2 table-elements inside a tr inside the tbody.
So the easy fix would be to remove the this: <th>{{ 'Status'|t }}</th>

But the better solution for me would be to remove the thead altogether and therefor the tbody-tag is also no longer needed. (the th-elements provide more than enough context & information)

Extras:
1) The RTL-styling was in comments because I wasn't sure how this should be on rtl-installs, but the screenshots made this more clear.

2) I was bothered by the fact that tr's without icons also had padding for the icon, so I fixed this. => This causes some wanted visual changes, so any opniion on this would be great.

3) Added a extra if statement to status-report.html.twig so the div with the status information is not printed for th's that don't need it.

4) @Droplet, wasn't sure why we should add a class on the td?

Ok cool, this is looking good. I'm not sure what the effects of removing the th's will have on screenreaders, so tagging this for accessibility review.

+++ b/core/modules/system/templates/status-report.html.twig
@@ -18,24 +18,20 @@
+      <tr class="system-status-report__entry system-status-report__entry--colored color-{{ requirement.severity_status }}">

I wonder if there is another class we could use instead of colored? I know that there is a color class being added but it would nice to keep the other classes logical, instead of presentational.

@LewisNyman not really sure if this is the tag you wanted?

Every <tr> now has the classes: system-status-report__entry--{{ requirement.severity_status }} This way, the classes are more consistent and more readable. The tr with the class system-status-report__entry--info is now the exception instead of the other 2 posiibilities in warnings.

Adjusted the css so nothing visually changes in comparison with last patch.

The interdiff is a bit chaotic because of tab-indents being altered :)

Ok thanks. This looks great I found one suggestion:

+++ b/core/modules/system/css/system.admin.css
@@ -188,24 +188,32 @@ small .admin-link:after {
+  padding: 10px 6px 10px 40px; /* LTR */
...
+[dir="rtl"] .system-status-report__status-title {
+  padding-left: 6px;
+  padding-right: 40px;

If we just set padding-left here then we don't have to override the vertical padding, which is nice.

Screenshot and bet eval added. I'll ping someone who can review the accessibility.

Good Work all, #34 looks good.
it is removing + padding-right: 40px; as it was suggested by @LewisNyman
Moving this to RTBC as of now.

Supplied a interdiff for #34.
This will still need some accessibility-review before it can go on rtbc.

+++ b/core/modules/system/css/system.admin.css
@@ -188,24 +188,31 @@ small .admin-link:after {
+  padding: 10px 6px 10px 40px; /* LTR */
...
+[dir="rtl"] .system-status-report__status-title {
+  padding-left: 6px;
 }

The padding-left is not applied for RTL. This results in the icon being displayed op top of the title.

Re: #35

it is removing + padding-right: 40px; as it was suggested by @LewisNyman
Moving this to RTBC as of now.

Lewis was suggesting only styling the padding-left instead of all the padding. The padding-right still needs to be applied for RTL.

@LewisNyman:
Not sure why you suggested to not override padding-right on RTL?
As screenshot shows, the padding stays necessary:

So everything inside #32 Is ok and there should the accessibility review should happen?

Extra: is the same interface being used on the install guide of Drupal? If so, it would be great if we can have some screenshots on those install pages, so we are sure everything looks ok there.

Not sure why you suggested to not override padding-right on RTL?
As screenshot shows, the padding stays necessary:

@MathieuSpil We should override the padding on RTL. I guess I worded it badly.

+.system-status-report__status-title {
...
+  padding: 10px 6px 10px 40px; /* LTR */
 }

We only need to set padding-left here, we don't need to set padding on the right, top, and bottom.

is the same interface being used on the install guide of Drupal? If so, it would be great if we can have some screenshots on those install pages, so we are sure everything looks ok there.

Ah good point, we should.

Updated css based upon #32, so it now only overrides padding-left on LTR and padding-right on RTL.

Tested it on the install steps, and it broke indeed.
I assumes there were 3 possible classes of the tr's:
- .system-status-report__status-icon--error => red
- .system-status-report__status-icon--warning => yellow
- .system-status-report__status-icon--error => default
But appareantly there is one more:
- .system-status-report__status-icon--ok => only used on the install pages as far as I can see.
As defined in:

/**
 * Prepares variables for status report template.
 *
 * Default template: status-report.html.twig.
 *
 * This theme function is dependent on install.inc being loaded, because
 * that's where the constants are defined.
 *
 * @param $variables
 *   An associative array containing:
 *   - requirements: An array of requirements/status items. Each requirement
 *     is an associative array containing the following elements:
 *     - title: The name of the requirement.
 *     - value: (optional) The current value (version, time, level, etc).
 *     - description: (optional) The description of the requirement.
 *     - severity: (optional) The requirement's result/severity level, one of:
 *       - REQUIREMENT_INFO: Status information.
 *       - REQUIREMENT_OK: The requirement is satisfied.
 *       - REQUIREMENT_WARNING: The requirement failed with a warning.
 *       - REQUIREMENT_ERROR: The requirement failed with an error.
 */
function template_preprocess_status_report(&$variables) {
  $severities = array(
    REQUIREMENT_INFO => array(
      'title' => t('Info'),
      'status' => 'info',
    ),
    REQUIREMENT_OK => array(
      'title' => t('OK'),
      'status' => 'ok',
    ),
    REQUIREMENT_WARNING => array(
      'title' => t('Warning'),
      'status' => 'warning',
    ),
    REQUIREMENT_ERROR => array(
      'title' => t('Error'),
      'status' => 'error',
    ),
  );

  foreach ($variables['requirements'] as $i => $requirement) {
    // Always use the explicit requirement severity, if defined. Otherwise,
    // default to REQUIREMENT_OK in the installer to visually confirm that
    // installation requirements are met. And default to REQUIREMENT_INFO to
    // denote neutral information without special visualization.
    if (isset($requirement['severity'])) {
      $severity = $severities[(int) $requirement['severity']];
    }
    elseif (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install') {
      $severity = $severities[REQUIREMENT_OK];
    }
    else {
      $severity = $severities[REQUIREMENT_INFO];
    }
    $variables['requirements'][$i]['severity_title'] = $severity['title'];
    $variables['requirements'][$i]['severity_status'] = $severity['status'];
  }
}

So tweaked the patch so now the warning and error are the exception. => CSS looks now more logical, while the twig looks a bit more complex (moved the if-statement one line higher)

After that took some screenshots, who do we address for accessibility?

Ah sorry. Needs a reroll

Just rerolled patch #42

After applying patch, design is looking same. Please find the attach below and after patch screenshots.

This patch looks good to me. Since it only changes CSS and HTML it's an unfrozen change, so should be fine for beta.

Before and after pics located in #46

+++ b/core/modules/system/templates/status-report.html.twig
@@ -18,29 +18,25 @@
+      {% if requirement.severity_status in ['warning', 'error'] %}

I don't think this if is the way to go here. The whole --has-icon looks like something we could do without.

Nice! Alex is right, we can just add icon classes to the th. One less div! I removed the requirement for the accessibiltiy review because the elements used haven't changed.

Is this visual change approved?

Before:

After:

@LewisNyman : What is the remaining work to do, as you pushed Needs Work ?

I don't see any visual change, so I try Needs Review again

@pguillard Sorry, the first column of the messages when warning or error are now bold, they weren't bold before. We shouldn't make any visual changes in this issue.

Thanks @LewisNyman
I replaced back <th> with the original <td>

@pguillard Thanks, I think we're close with this. I think we need to keep the <th> for accessibility reasons. Is there a way we could just style the th so it doesn't include the bold styling?

@LewisNyman Just re-rolled #56 as per your suggestion. Please see if that is correct. Attaching screenshot.

Ohk. My patch in #58 didn't capture everything properly. I updated the patch. Apologies for that. Screenshot remains the same as #58 though.

Checking #61 in order to set it to RTBC, I discovered the patch was not applying anymore, just rerolled it.
Screenshot remains the same as #58.

Great, thanks.

The alignment of the text and icons has changed - the icons used to be in a "column" of their own.

To see what I mean look at #51 thanks @lauriii and @emma

Ok, will do.

I can help to visually test the solution when it is ready also :-)

Here is a update.

Thanks, this looks better.

+++ b/core/modules/system/css/system.admin.css
@@ -192,16 +192,12 @@ small .admin-link:after {
+  padding: 10px 6px 10px 40px;

Needs LTR thingy. Will add on commit.

There is one already.

CSS and Twig only - permitted in beta. Committed eb6330d and pushed to 8.0.x. Thanks!

@mansspams this is what I meant...

diff --git a/core/modules/system/css/system.admin.css b/core/modules/system/css/system.admin.css
index db8bb09..b01e405 100644
--- a/core/modules/system/css/system.admin.css
+++ b/core/modules/system/css/system.admin.css
@@ -192,7 +192,7 @@ small .admin-link:after {
   position: relative;
   vertical-align: top;
   width: 25%;
-  padding: 10px 6px 10px 40px;
+  padding: 10px 6px 10px 40px; /* LTR */
   box-sizing: border-box;
   font-weight: normal;
 }

Fixed on commit.