Move miscellaneous system classes from preprocess to templates

This patch is pretty much doing what was needed. Some classes were left behind in template_preprocess_system_themes_page in screenshots and links theme arrays. I don't really know what to do with that kind of usage situation.

Thanks @lanchez, this was a tricky one. Here's an initial review. My overall feeling is I think we may want to scale back a bit (mostly thinking of the status report) but if other folks are on board that's fine with me.

1. +++ b/core/modules/system/system.admin.inc
   @@ -53,11 +53,10 @@ function _system_is_incompatible(&$incompatible, $files, Extension $file) {
        $compact = system_admin_compact_mode();
   ...
   +    $variables['compact'] = ($compact) ? TRUE : NULL;
   

   Why not $variables['compact'] = $compact; here? system_admin_compact_mode() returns a boolean.

2. +++ b/core/modules/system/system.admin.inc
   @@ -374,7 +373,7 @@ function template_preprocess_system_themes_page(&$variables) {
   -    $theme_group['attributes'] = new Attribute(array('class' => array('system-themes-list', 'system-themes-list-' . $state, 'clearfix')));
   
   +++ b/core/modules/system/templates/system-themes-page.html.twig
   @@ -28,10 +30,25 @@
   +        'system-themes-list-' ~ theme_group.state|clean_class,
   

   $state didn't go through a CSS-ifier before, not sure it should after.

3. +++ b/core/modules/system/templates/admin-block-content.html.twig
   @@ -9,14 +9,21 @@
   + * - compact: This is true, if compact mode is turned on.
   
   +++ b/core/themes/seven/templates/admin-block-content.html.twig
   @@ -11,13 +11,20 @@
   + * - compact: This is true, if compact mode is turned on.
   

   I think we can lose the comma at least.

4. +++ b/core/modules/system/templates/status-report.html.twig
   @@ -10,13 +10,26 @@
   - *   - severity_class: The HTML class of the severity.
   + *   - severity_status: Indicates the severity status. This can be:
   + *     * info
   + *     * ok
   + *     * warning
   + *     * error
   

   Maybe "This can be one of:" and the list items should use hyphens instead of asterisks per https://www.drupal.org/node/1354#lists.

5. +++ b/core/modules/system/templates/status-report.html.twig
   @@ -10,13 +10,26 @@
   +{# severity_status : class #}
   +{%
   +  set severities = {
   +    'info' : 'info',
   +    'ok' : 'ok',
   +    'warning' : 'warning',
   +    'error' : 'error',
   +  }
   +%}
   

   I think the code comment here confused me, if we want to document this I think it should be a complete sentence explaining the array. Should be no space between the array key and the colon per Twig coding standards: http://twig.sensiolabs.org/doc/coding_standards.html

6. +++ b/core/modules/system/templates/system-themes-page.html.twig
   @@ -28,10 +30,25 @@
   +    <div {{ theme_group.attributes.addClass(theme_group_classes) }}>
   

   Extra space between <div and {{ here.

Thanks for review!

I will make another patch, but one comment on system_admin_compact_mode(). It does not work right right now, since it really returns the value of the cookie (string). So no boolean there and thats why the workaround is in place. The D7 version of the function works fine.

I did a few modifications:
1. Didn't yet change that yet -> could someone confirm if system_admin_compact_mode() works as intended or not?
2. Removed clean_class
3. Fixed comment to: - compact: True if compact mode is turned on.
4/5. Removed whole array. I don't even remember why I did it that way. Now it just prints severity_status|clean_class
6. Removed space.

@lanchez, thanks. That's looking much better to me! For #1 adding a code comment to explain would be good for the scope of this issue and we can check for an existing issue or file a new one for the incorrect behaviour/docs.

1. +++ b/core/modules/system/templates/admin-block-content.html.twig
   @@ -9,7 +9,7 @@
   - * - compact: This is true, if compact mode is turned on.
   + * - compact: True if compact mode is turned on.
   
   +++ b/core/themes/seven/templates/admin-block-content.html.twig
   @@ -11,7 +11,7 @@
   - * - compact: This is true, if compact mode is turned on.
   + * - compact: This is true if compact mode is turned on.
   

   I really like the the "True if…" one better! Can we make them both consistent please?

2. +++ b/core/modules/system/templates/status-report.html.twig
   @@ -38,7 +25,7 @@
   -    <tr class="{{ severities[requirement.severity_status] }}">
   +    <tr class="{{ severity_status|clean_class }}">
   

   Doesn't this need to be requirement.severity_status? Again, not sure if we need clean_class in this case.

Doh, stupid mistakes from me :)

I fixed the comment and severity_status class (and removed the clean_class, since its a manual string in code). Also added the comment you suggested.

Nice work on the patch!

+++ b/core/modules/system/system.admin.inc
@@ -53,11 +53,12 @@ function _system_is_incompatible(&$incompatible, $files, Extension $file) {
+    // Check if compact mode is turned on.
+    // NOTICE: system_admin_compact_mode() seems to return a string
+    // and not a boolean.
     $compact = system_admin_compact_mode();
-    $variables['attributes'] = array('class' => array('admin-list'));
-    if ($compact) {
-      $variables['attributes']['class'][] = 'compact';
-    }
+    $variables['compact'] = ($compact) ? TRUE : NULL;

What are we trying to tackle exactly with this change? I think we instead need to fix the function or documentation for the function.

Reviewed, nice work :)

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -15,6 +15,10 @@
+ *     - is_default: Boolean indicating whether the theme is default theme or
...
+ *     - is_admin: Boolean indicating whether the theme is the admin theme or
+ *       not.

Change to: is_default: Boolean indicating whether the theme is the default theme not.

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -9,14 +9,21 @@
+ * - compact: Boolean indicating whether compact mode is turned on or not.

+++ b/core/themes/seven/templates/admin-block-content.html.twig
@@ -11,13 +11,20 @@
+ * - compact: True if compact mode is turned on.

I would change the second documentation part to be the same as the first one. That would make sense, right? True if compact mode is turned on. would then just become Boolean indicating whether compact mode is turned on or not., like the first message.

Tagging this as needs works again, removing some of the tags and adding the Novice tag, since fixing the remaining things would be a novice task.

Patch Reroll and comments fixed.

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -15,6 +15,10 @@
+ *     - is_default: Boolean indicating whether the theme is the default theme or
+ *       not.

Thanks Jolidog for fixing the documentation. I've got to nitpick a little more… The or should be on the next line, according to the Drupal coding standards; since it's exceeding the 80 characters per line limit.

Thanks sqndr for spotting this, "or" is now on the next line.

Doing manual review whit Kaleidoscope.

I manually reviewed the patch to verify the classes still appear where they should and nothing is broken. BANANA!

Committed 79f952a and pushed to 8.0.x. Thanks!

Putting back to fixed state