Move table classes from preprocess to templates

What should we do with classes that are coming from constants e.g.RESPONSIVE_PRIORITY_MEDIUM?

Here's one option: http://twig.sensiolabs.org/doc/functions/constant.html

Thanks @Cottser! My question might have been irrelevant because after all I thought we might not even want to move those from preprocess to Twig. I applied my patch where all of the classes excluding responsive classes have been moved from preprocess to Twig.

+++ b/core/modules/system/templates/table.html.twig
@@ -58,7 +58,7 @@
+          <{{ cell.tag }}{{ cell.attributes.addclass(cell.table_sort ? 'active') }}>

@@ -69,9 +69,14 @@
+            <{{ cell.tag }}{{ cell.attributes.addClass(empty ? ['empty', 'message']) }}>

I think we've been saying these should go in a set tag because they contain logic.

And cool that you didn't need to worry about those responsive classes! I guess that's probably something that should stay baked in anyway…

Created new patch which should pass the tests. Template was spitting empty class attributes which failed the tests.

Thanks @davidhernandez! That was very annoying problem! I uploaded new version of the patch and it most likely will fail the tests.

I think this one is blocked as well.

Reviewed and tested, looks good to me :)

The documentation in table.html.twig needs updating for the new variables.

Removing some of the tags. Found a typo in the documentation, updating the patch and added an interdiff.

I just had a look at the patch and tested the whole thing with the latest DEV - looks fine for me.

Putting back to RTBC

Minor docs tweak, was going to leave at RTBC, but…

+++ b/core/includes/theme.inc
@@ -1311,17 +1310,14 @@ function template_preprocess_table(&$variables) {
-        $no_striping = $section === 'footer';

I'm concerned we're losing this logic. Where has this been moved to?

Nevermind, the footer is a separate part of the template :) back to RTBC.

1. +++ b/core/includes/theme.inc
   @@ -1244,7 +1244,6 @@ function template_preprocess_table(&$variables) {
        $variables['rows'][] = array(array(
          'data' => $variables['empty'],
          'colspan' => $header_count,
   -      'class' => array('empty', 'message'),
        ));
   
   +++ b/core/modules/system/templates/table.html.twig
   @@ -69,9 +76,20 @@
   +            {%
   +              set cell_classes = [
   +                empty ? 'empty',
   +                empty ? 'message',
   +              ]
   +            %}
   +            <{{ cell.tag }}{{ cell.attributes.addClass(cell_classes) }}>
   

   Maybe it would be possible to not add these classes in a loop if we didn't add the empty message as a row in preprocess.

2. +++ b/core/includes/theme.inc
   @@ -1371,7 +1360,7 @@ function template_preprocess_table(&$variables) {
   -              $cell_attributes['class'][] = 'active';
   +              $variables[$section][$row_key]['cells'][$col_key]['table_sort'] = TRUE;
   
   +++ b/core/modules/system/templates/table.html.twig
   @@ -28,9 +28,11 @@
   + *     - active: A boolean indicating whether the cell is active.
   
   @@ -58,7 +60,12 @@
   +            set cell_classes = [
   +              cell.table_sort ? 'active',
   +            ]
   

   I think the variable name is table_sort. I think a better name might be active_table_sort.

There is still the row loop - we could something like this.

Looks fine to me - no visual problems.

Empty row is spanning the correct number of columns and row attributes are correct.

+++ b/core/modules/system/templates/table.html.twig
@@ -69,7 +78,12 @@
+            not no_striping ? cycle(['odd', 'even'], loop.index0),

Don't love not no_striping but not going to not commit this over the double negative.

Committed/pushed to 8.0.x, thanks!