Move views classes from preprocess to templates

Here's my progression on this one. There's still a lot to do so I'm just sending this to test bot.

Unassigning myself. I will continue working with this one but anyone who feels like grabbing this, go ahead!

There's still at least template_preprocess_views_mini_pager which I didn't do because of #2318341: Views mini pager markup

This should fix the tests

Removed debugging line

I think this is looking really good, thank you @lauriii!

+++ b/core/modules/views/templates/views-view-grid.html.twig
@@ -38,11 +63,28 @@
     {% for row in column.content %}
-      <div{{ row.attributes }}>
-        {{ row.content }}
-      </div>
+    {% if options.row_class_default %}
+      {%
+        set row_classes = [
+          'views-row',
+          'row-' ~ loop.index,
+        ]
+      %}
+    {% endif %}
+    <div{{ row.attributes.addClass(row_classes) }}>
+      {{ row.content }}
+    </div>
     {% endfor %}

Minor but can we maintain the indent here please?

Now the indentations should be good!

Reviewed – good work, and the indentation is now correct.

Just curious whether this was discussed properly, which I don't see here, sadly.

1. +++ b/core/modules/views/templates/views-view-grid.html.twig
   @@ -22,15 +22,40 @@
   +  {% if options.row_class_default %}
   +    {%
   +      set row_classes = [
   +        'views-row',
   +        'row-' ~ loop.index,
   +        'clearfix',
   +      ]
   +    %}
   
   @@ -38,12 +63,29 @@
   +        {% if options.row_class_default %}
   +          {%
   +            set row_classes = [
   +              'views-row',
   +              'row-' ~ loop.index,
   +            ]
   +          %}
   
   +++ b/core/modules/views/views.theme.inc
   @@ -703,14 +687,6 @@ function template_preprocess_views_view_grid(&$variables) {
   -      if ($options['row_class_default']) {
   -        $row_attributes['class'][] = 'views-row';
   -        $row_attributes['class'][] = 'row-' . ($row + 1);
   -        if ($horizontal) {
   -          $row_attributes['class'][] = 'clearfix';
   -        }
   -      }
   

   So previously we had one place for this logic, now there are two. How is that a good idea?

2. +++ b/core/modules/views/templates/views-view-grid.html.twig
   @@ -22,15 +22,40 @@
   +    {% if options.col_class_default %}
   +      {%
   +        set col_classes = [
   +          'views-col',
   +          'col-' ~ loop.index,
   +        ]
   +      %}
   
   @@ -38,12 +63,29 @@
   +      {%
   +        set col_classes = [
   +          'views-col',
   +          'col-' ~ loop.index,
   +          'clearfix',
   +        ]
   
   +++ b/core/modules/views/views.theme.inc
   @@ -729,13 +705,6 @@ function template_preprocess_views_view_grid(&$variables) {
   -      if ($options['col_class_default']) {
   -        $col_attributes['class'][] = 'views-col';
   -        $col_attributes['class'][] = 'col-' . ($col + 1);
   -        if (!$horizontal) {
   -          $col_attributes['class'][] = 'clearfix';
   -        }
   -      }
   

   Same, but different.

3. +++ b/core/modules/views/views.theme.inc
   @@ -28,13 +28,6 @@ function template_preprocess_views_view(&$variables) {
      $css_class = $view->display_handler->getOption('css_class');
      if (!empty($css_class)) {
        $variables['css_class'] = preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class);
   

   Same question here.

4. +++ b/core/modules/views/views.theme.inc
   @@ -333,10 +325,10 @@ function template_preprocess_views_view_summary(&$variables) {
        if (isset($active_urls[$variables['rows'][$id]->url])) {
   -      $variables['rows'][$id]->attributes['class'][] = 'active';
   +      $variables['rows'][$id]['active'] = TRUE;
        }
   
   @@ -398,7 +390,7 @@ function template_preprocess_views_view_summary_unformatted(&$variables) {
        if (isset($active_urls[$variables['rows'][$id]->url])) {
   -      $variables['rows'][$id]->attributes['class'][] = 'active';
   +      $variables['rows'][$id]['active'] = TRUE;
        }
   

   Write this shit in one line, please

5. +++ b/core/modules/views/views.theme.inc
   @@ -801,14 +770,11 @@ function template_preprocess_views_view_unformatted(&$variables) {
        if ($row_class = $view->style_plugin->getRowClass($id)) {
          $variables['rows'][$id]['attributes']['class'][] = $row_class;
        }
   

   so wait what? Why do we not add this into the template?

Fixing tests

didnt remember that row classes can have tokens inside

#15.1 and #15.2 should still be checked

Tested few different options to clean up #15.1 and #15.2 and didn't find anything dramatically cleaner that would fit into this issues scope.

Reroll

Unless testbot doesn't agree, this looks RTBC.

Rerolled

Rerolled and fixed some documentation

Added documentation to responsive variable

In views-view.html.twig, if the id is being passed to the clean_class filter on line 43, should it not also be passed elsewhere? Is this actually needed here?

+  set classes = [
+    'view',
+    'view-' ~ id|clean_class,
+    'view-id-' ~ id,
+    'view-display-id-' ~ display_id,
+    dom_id ? 'view-dom-id-' ~ dom_id,
+  ]

Still some classes being added in preprocess functions, e.g:

template_preprocess_views_view_table()

      // Set up the header label class.
      $variables['header'][$field]['attributes'] = array();
      if ($fields[$field]->options['element_default_classes']) {
        $variables['header'][$field]['attributes']['class'][] = 'views-field';
        $variables['header'][$field]['attributes']['class'][] = 'views-field-' . $variables['fields'][$field];
      }

template_preprocess_views_view_list()

  // Fetch wrapper classes from handler options.
  if ($handler->options['wrapper_class']) {
    $wrapper_class = explode(' ', $handler->options['wrapper_class']);
    $variables['attributes']['class'] = array_map('drupal_clean_css_identifier', $wrapper_class);
  }

I think we have to move a bit backwards with this. We cannot move the Views UI html class functionality to templates because it would break if someone removes the variables from the templates. E.g moving the classes to the classy would break it. Different question is that if we need it anymore in Views UI that is in core but it shouldn't be discussed here.

I haven't seen any discussion on d.o regarding such matter but we've had some unofficial discussions about that. I guess we are too late to get that into D8.

Anyhow I moved table default classes to template and added some more documentation for missing parts.

I diffed the html output in HEAD and with the patch and the only difference is the order of the attributes and classes, which is fine. It looks like the php has already been reviewed. RTBC!

So are there no answers to #15.1 and #15.2?

If I remember correctly we discussed this with dawehner in Amsterdam and came into consensus that refactoring that isn't in the scope of this issue. We could instead create follow-up to fix it.

+++ b/core/modules/views/templates/views-view-summary-unformatted.html.twig
@@ -20,11 +21,16 @@
 {% for row in rows  %}
+  {%
+    set row_classes = [
+      row.active ? 'active',
+    ]
+  %}
   {{ options.inline ? '<span' : '<div' }} class="views-summary views-summary-unformatted">
   {% if row.separator -%}
     {{ row.separator }}
   {%- endif %}
-  <a href="{{ row.url }}"{{ row.attributes }}>{{ row.link }}</a>
+  <a href="{{ row.url }}"{{ row.attributes.addClass(row_classes) }}>{{ row.link }}</a>

This kind of bothers me. We're in a loop - one row is probably going to be active and we're creating and adding an empty class to all the other rows. How about doing something like

    {% if row.active %}
      {% row.attributes.addClass('active') %}
    {% endif %}

instead. We have lots of stuff occurring in loops here and I think this kind of optimisation could be used elsewhere in the patch.

Also this issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. Obviously as part of the banana consensus it can go in but it's nice to have the justification in a standard way on every issue.

Fixes #44

One question here: Shouldn't we change 'active' class to 'is-active'?
There is already issue to change all classes in core #2031641: Change active class to is-active

@Dragan Eror: I don't think its in the scope of this issue

It's not possible to modify attributes object there because its protected. Tried something Cottser suggested to try.

Ignore latest patch :P

+++ b/core/modules/views/templates/views-view-summary.html.twig
@@ -22,7 +23,12 @@
-    <li><a href="{{ row.url }}"{{ row.attributes }}>{{ row.link }}</a>
+    {%
+      set row_classes = [
+        row.active ? 'active',
+      ]
+    %}
+    <li><a href="{{ row.url }}"{{ row.attributes.addClass(row_classes) }}>{{ row.link }}</a>

@lauriii I really like the clean-up from the last patch, could you do that here too?

Setting to needs work based on joel's comments

Yeah that's a good catch

Rerolled to include small coding style changes where the new variables are documented (to follow correct style from https://www.drupal.org/node/1354#lists).

Reviewed for:

Consistent changes to templates

• Available variables list
• Define default classes arrays (classes, row_classes, col_classes)
• Update wrappers to add default classes, if enabled (classes, row_classes, col_classes)

Consistent changes to summary templates

• Addition of active class

Consistent changes to preprocess functions

• Removal of $variables['attributes']['class']es

Summary of open concerns addressed

• Streamline conditional addition of active class
  Issue raised in #44, reminded in #54, resolved in #56.
• Splitting of where classes are defined in the template file
  Raised in #15.1/#15.2, rephrased in #45, declared as out-of-scope in #43 (follow-up issue recommended?)

Reviewing against list of functions/templates intended to be modified

                                  .html.twig      preprocess
views-view                            ✓               ✓
views-view-fields                     1a              1b
views-view-summary                    ✓               ✓
views-view-summary-unformatted        ✓               ✓
views-view-table                      ✓               ✓
views-view-grid                       ✓               ✓
views-view-unformatted                ✓               ✓
views-view-list                       2a              2b
views-mini-pager                      3               ✓

1. a. views-view-fields.twig.html contains no attributes declarations, but
   b. template_preprocess_views_view_fields() has $attributes['class'].
2. a. views-view-list.twig.html contains attributes declarations, and
   b. template_preprocess_views_view_list() has $variables['attributes']['class'].
3. views-mini-pager.twig.html has lots of classes defined, but not in variables.

Didn't mean to remove sprint tag. Setting needs work for feedback on 1-3.

I think the patch didn't support multifield columns. I tried to fix that.

Compared markup being printed out before vs after the patch, on these situations (with generated content):
OK = identical markup

• Default homepage view (Unformatted list) - OK
• Table view with 3 fields - OK
• Grid view with Teasers - OK

laurii's change on #63 to previous patch does the trick for multifield columns.

However, I'm setting back to needs work because we still have to work on missing parts of this patch, quoting from #58:

1. a. views-view-fields.twig.html contains no attributes declarations, but
   b. template_preprocess_views_view_fields() has $attributes['class'].
2. a. views-view-list.twig.html contains attributes declarations, and
   b. template_preprocess_views_view_list() has $variables['attributes']['class'].
3. views-mini-pager.twig.html has lots of classes defined, but not in variables.

About 1:

views-view-fields.twig.html contains no attributes declarations, but template_preprocess_views_view_fields() has $attributes['class'].

The preprocess function in this case is actually building the markup that the user has configured through the admin UI. I agree with @davidhernandez, unless we want to do all the processing on the twig file, we should just leave it be there. The $attributes['class'] array gets processed directly in there $attributes = new Attribute($attributes); to build up the markup string for the html tag, as configured by the user.

Only thing that we could do to improve themers' lives would be to think about the element_default_classes: 'field-content', 'views-field' and 'views-label'. But then again you can swap these out by de-selecting the add default classes options. Not sure it'd be worth the effort.

And I think the same case applies for item 2:

views-view-list.twig.html contains attributes declarations, and template_preprocess_views_view_list() has $variables['attributes']['class'].

The classes here are also coming from Views UI user configuration.

We will have things that comes from the UI, thats "unfortunately" But its not our role to take that discussion + that ship have sailed & its a timesuck we dont wanna get into.

Make sure that the UI can add classes/whatever as a user wanna add in through the ui its a feature (not a bug...)
as david says its ok + its a huge step forward from what we have now.

views mini-pager should have all those classes, so its the same structure as pager.html.twig, in the "move templates to classy & cleanup core for not needed classes" follow up issue, we will take care of cleaning classes.

Renaming of classes & what they should be called will first happen when templates are in classy.

Alright then all is good to go now that we have cleared out the last 3 outstanding points in #65, #66 and #67.

The patch still applies fine, so RTBC.

+++ b/core/modules/views/templates/views-view-grid.html.twig
@@ -22,15 +26,40 @@
+  <div{{ row.attributes.addClass(row_classes) }}>
...
+    <div{{ column.attributes.addClass(col_classes) }}>

@@ -38,12 +67,29 @@
+    <div{{ column.attributes.addClass(col_classes) }}>

What happens when these template variables are not set? E.g. col_class_default is FALSE.

This looks great, compared the templates with the corresponding preprocess functions ... and they look alright.

1. +++ b/core/modules/views/templates/views-view-summary-unformatted.html.twig
   @@ -9,6 +9,7 @@
   + *   - active: A flag indicating whtether the row is active.
   

   typo

2. +++ b/core/modules/views/views.theme.inc
   @@ -807,18 +771,15 @@ function template_preprocess_views_view_unformatted(&$variables) {
   +  $variables['default_row_class'] = isset($options['default_row_class']) ? $options['default_row_class'] : FALSE;
   

   Afaik you can just use !empty($options['default_row_class']) to simplify this row of code.

For #70 and it'd be nice to get answer to #69 - I think the answer is twig does not care.

@damiankloip (#69) - The markup outputted in the case you describe is exactly the same before applying this patch than after.

You basically get this as a wrapper for the item: <div style="width: 25%;"> (for 4 columns).

Without the patch:

With the patch applied (and cache rebuilt):

Working on #70

Fixing what was mentioned on #70, interdiff against #63.

@dawehner mentioned in #15 this hunk. Has anybody tested this "horizontal"?

+++ b/core/modules/views/views.theme.inc
@@ -735,13 +706,6 @@ function template_preprocess_views_view_grid(&$variables) {
-      if ($options['col_class_default']) {
-        $col_attributes['class'][] = 'views-col';
-        $col_attributes['class'][] = 'col-' . ($col + 1);
-        if (!$horizontal) {
-          $col_attributes['class'][] = 'clearfix';
-        }

missing clearfix for horizontal to function?

Sorry for wet blanketing the RTBC, this patch looks great so far!

I like it @davidhernandez, it's a lot more clear to read, and work with.

I tested it manually and the classes are still added properly for vertical & horizontal view displays.

If we were to make it even easier, I think we could perhaps split the file into two, one for vertical and another for horizontal:

• views-view-grid-vertical.html.twig
• views-view-grid-horizontal.html.twig

Although that would mean duplicating code a bit, so I'm not convinced with the idea.
What do you think? Is this clear enough for themers, or should we try to go the extra mile?

dont split em - thats overkill.

@davidhernandez - all good points

OK, let's get this one in!

Committed 5f05852 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

so this was banana phase 1 ?
*dance*

Awesome!!

Next:

1. #2424533: Copy views templates to Classy
2. #2349775: Remove classes from Views templates