Move classes out of the preprocess functions and into the Twig templates. Use the addClass() attribute method to add classes in the template. Use the clean_class filter to filter class names, if necessary. Maintain all existing functionality and ensure all existing class names are still in the markup, even ones that are inherited.

See the following issues for more detailed examples:
#2217731: Move field classes out of preprocess and into templates
#2254153: Move node classes out of preprocess and into templates

See this change record for information about using the addClass() method:
https://www.drupal.org/node/2315471

See this change record for more information about the phase 1 process of moving class from preprocess to templates:
https://www.drupal.org/node/2325067

Preprocess Functions Modified

template_preprocess_table

Twig Templates Modified

table.html.twig

Comments

davidhernandez’s picture

Issue tags: +FUDK
lauriii’s picture

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

Cottser’s picture

lauriii’s picture

Status: Active » Needs review
FileSize
3.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,837 pass(es), 29 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 4: move_table_classes_from-2329767-4.patch, failed testing.

Cottser’s picture

+++ 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.

Cottser’s picture

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…

lauriii’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,865 pass(es). View

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

davidhernandez’s picture

Status: Needs review » Needs work

@laurii, those ifs are not needed. We know about the empty class issue and that is being resolved here. #2330731: Attribute::addClass() adds empty class

lauriii’s picture

Status: Needs work » Needs review
FileSize
3.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,315 pass(es). View
1.71 KB

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

Status: Needs review » Needs work

The last submitted patch, 10: move_table_classes_from-2329767-10.patch, failed testing.

davidhernandez’s picture

Cottser’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: move_table_classes_from-2329767-10.patch, failed testing.

Cottser’s picture

Status: Needs work » Postponed
Related issues: +#2336355: Refactor Attribute rendering so that class="" is not printed

I think this one is blocked as well.

davidhernandez’s picture

lauriii’s picture

FileSize
3.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,654 pass(es). View
1.02 KB
mikl’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested, looks good to me :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
741 bytes
4.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,797 pass(es). View
sqndr’s picture

Issue tags: -frontendunited, -FUDK
FileSize
4.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,861 pass(es). View
598 bytes

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

SteffenR’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: move_table_classes_from-2329767-23.patch, failed testing.

The last submitted patch, 23: move_table_classes_from-2329767-23.patch, failed testing.

The last submitted patch, 23: move_table_classes_from-2329767-23.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

Putting back to RTBC

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,027 pass(es). View
714 bytes

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?

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
4.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,019 pass(es). View
alexpott’s picture

FileSize
5.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,495 pass(es). View
3.36 KB

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

jamesquinton’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine to me - no visual problems.

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

catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ 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!

  • catch committed 5f47f20 on 8.0.x
    Issue #2329767 by lauriii, alexpott, Cottser, sqndr: Move table classes...
lauriii’s picture

FileSize
42.44 KB

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.