Rewrite the system status report component inline with our CSS standards

Hi LewisNyman,

Just started looking at this and begun with the status report system table.

Here is a first patch just for that component.

Some considerations:

• In terms of markup, I added some sub-component selectors to the status report table component .system-status-report.These components are:
  • The header .system-status-report__header
  • The body .system-status-report__body
  • Each status report entry system-status-report__entry
  • Each status report status entry icon .system-status-report__status-icon
  • Each status report status entry title .system-status-report__status-title
  • Each status report status entry value .system-status-report__status-value
• I also considered the following states (severity):
• The error .is-error
• The warning .is-warning
• The info .is-info

For this I changed the respective twig file, seven tables.css colors.css (these should probably be moved to a states like states.css or severity.css?) and system system.admin.css.

Also, in regards to the states it feels redundant to have the error state on the entry as well as the status icon. My suggestion is to have it only on the parent but I didn't want to patch it before discussing that.

Let me know if I am on the right track and I will continue working on it. I am still getting used to the new standards as they are, so any guidance is welcome :)

Thanks,

Hi LewisNyman,

Thanks for your review!

Makes sense that they are only used as states when used in the component, that was actually where I was heading, however given the https://www.drupal.org/node/2336141 I agree we can maintain them as separate classes.

1. I assume the reason for that div is to have a placeholder for the icon that sits inside the padded area and has the icon's image dimensions. The way to avoid this I supposed could be to somehow use the background-image in status-icon itself, by using for example, a pseudo element like :before.
2. Agreed, will fix.
3. OK
4. We don't t the moment, however if a theme needs to style those columns one would need to override the twig file to add classes in order to avoid using base elements. If this is the preferred approach I will get ride of them.
5. Same as above.
6. Agree 100%.

Will provide a patch with all these changes and get back to you.

Cheers,

Great,

Will give you a patch for everything tomorrow.

Hey,

Ok so I took care of all the issues we identified plus a couple of another ones.

The one thing that is still annoying me is the need for the .system-status-report td in system.admin.css to make the vertical-align:top.

It would be really good to get read of it, but we need the align top here for readability.

Any suggestions?

Forgot to attach the screens.

1. +++ b/core/modules/system/css/system.admin.css
   @@ -187,37 +187,33 @@ small .admin-link:after {
    .system-status-report td {
   -  padding: 6px;
      vertical-align: top;
    }
   -.system-status-report td:nth-child(-n+2) {
   -  background-color: rgba(0, 0, 0, 0.04);
   -}
   

   This styling is actually used currently in Stark. Is there a reason why this should be removed?

2. +++ b/core/modules/system/css/system.admin.css
   @@ -187,37 +187,33 @@ small .admin-link:after {
   +.system-status-report__status-icon:before {
   +  content:"";
   

   According to Drupal CSS coding standards every declaration should be followed by a single space, eg. use 'content: ""', instead of 'content:""'.

Hi idebr,

Thanks for your review!

1. This seems very specific to be on the system.admin.css. It's setting the first column to grey which for me doesn't make sense from a generic context and will imply all themes to override it. Understood it's being used in Stark but shouldn't it be applied to Stark instead then?
2. My bad, will put up a patch later tonight for this.

Let me know what you guys think regarding 1)

Attached path and interdiff as per #9.1

There are some design differences here, but not user facing

Well it is admin user facing if using stark. This change makes the status report much harder to read in stark.

Also can we get screenshots of this in seven

+++ b/core/themes/seven/seven.libraries.yml
@@ -26,6 +26,7 @@ global-styling:
+      css/components/system-status-report.css: {}

Doesn't this mean we should have a screenshots of seven?

Hey,

Sorry guys my bad forgot to add the system status report css file to the commit.

Attaching correct patch along with before and after screens.

The intention of Stark is not to provide a useable interface, it's a functional base theme. It's only selectable as an active theme as a demonstration of Drupal's default output. I think that this issue is just tidying up the separation between the module CSS and the theme CSS.

I had a second look after I noted this change earlier in #9 and I have to agree with @LewisNyman here. The styling is too opinionated to apply for all themes and Stark has no theme css of its own, so let's remove it entirely.

One thing I did not see mentioned:

+++ b/core/themes/seven/css/components/tables.css
@@ -124,26 +126,6 @@ th.select-all {
-table.system-status-report td {
-  padding: 12px;
-}

This styling is removed, but never reapplied. This makes the status report a little more dense in Seven. I'll leave this up to @LewisNyman if this is ok.

Beauty RTBC+1

We should have a CR to tell people about the new classes - there is a change record somewhere that is detailing changes to classes for 8.0.x

Hi @alexpott I didn't find a generic CR like that.

What I found are several CRs for different CSS changes.

For the moment at least I created a CR for the system status component which would incorporate any changes to this component (same as for the help component).

Thank you @kyuubi. That looks like it has all the bits.

https://www.drupal.org/node/2464095

CSS is not frozen in beta. Committed 3d01747 and pushed to 8.0.x. Thanks!

Looks like the change notice has the class names backwards between D7 and D8

Hey @nod_,

Thanks for that, honestly dunno what happened there.

Fixed.

thanks @nod_ and @kyuubi I should have spotted that too:(