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_views_view
template_preprocess_views_view_fields
template_preprocess_views_view_summary
template_preprocess_views_view_summary_unformatted
template_preprocess_views_view_table
template_preprocess_views_view_grid
template_preprocess_views_view_unformatted
template_preprocess_views_view_list
template_preprocess_views_mini_pager

Twig Templates Modified

views-view.html.twig
views-view-fields.html.twig
views-view-summary.html.twig
views-view-summary-unformatted.html.twig
views-view-table.html.twig
views-view-grid.html.twig
views-view-unformatted.html.twig
views-view-list.html.twig
views-mini-pager.html.twig

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves the Themer Experience.
Issue priority Normal.
Unfrozen changes Unfrozen because it changes markup.
Prioritized changes The main goal of this issue is usability of the new Twig theming layer, and since it adjusts markup it must be completed prior to RC.
Disruption This issue is a prioritized change (theme improvements) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption.

Files: 
CommentFileSizeAuthor
#80 interdiff-79to80.txt2.88 KBdavidhernandez
#80 move_views_classes_from-2329917-80.patch18.76 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,300 pass(es). View
#79 move_views_classes_from-2329917-79.patch19.09 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,320 pass(es). View
#73 interdiff.txt1.38 KBManuel Garcia
#73 move_views_classes_from-2329917-73.patch18.77 KBManuel Garcia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,258 pass(es). View
#72 views-classes-with-patch.png10.71 KBManuel Garcia
#72 views-class-before-patch.png9.76 KBManuel Garcia
#63 interdiff.txt1.43 KBlauriii
#63 move_views_classes_from-2329917-63.patch18.81 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,259 pass(es). View
#58 interdiff-2329917-56-58.txt1.75 KBakalata
#58 2329917-move_views_classes-58-coding-style.patch18.53 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,994 pass(es), 1 fail(s), and 0 exception(s). View
#58 2329917-move_views_classes-58-rerolled.patch18.52 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,996 pass(es), 1 fail(s), and 0 exception(s). View
#56 move_views_classes_from-2329917-56.patch18.46 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,569 pass(es). View
#56 interdiff.txt713 byteslauriii

Comments

davidhernandez’s picture

Issue tags: +FUDK
lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

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

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

lauriii’s picture

Assigned: lauriii » Unassigned

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

Status: Needs review » Needs work

The last submitted patch, 3: move_views_classes_from-2329917-3.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,314 pass(es), 16 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 6: move_views_classes_from-2329917-6.patch, failed testing.

lauriii’s picture

FileSize
13.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,330 pass(es). View

This should fix the tests

lauriii’s picture

Status: Needs work » Needs review
lauriii’s picture

FileSize
549 bytes
12.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,256 pass(es). View

Removed debugging line

Cottser’s picture

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?

lauriii’s picture

FileSize
1.43 KB
12.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,704 pass(es). View
lauriii’s picture

FileSize
12.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,704 pass(es). View
1016 bytes

Now the indentations should be good!

mikl’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

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?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
lauriii’s picture

Status: Needs work » Needs review
FileSize
12.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,712 pass(es), 19 fail(s), and 6 exception(s). View
2.23 KB

Status: Needs review » Needs work

The last submitted patch, 17: move_views_classes_from-2329917-17.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
12.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,725 pass(es), 5 fail(s), and 0 exception(s). View
1.09 KB

Fixing tests

Status: Needs review » Needs work

The last submitted patch, 19: move_views_classes_from-2329917-19.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1004 bytes
13.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch move_views_classes_from-2329917-21.patch. Unable to apply patch. See the log in the details link for more information. View

didnt remember that row classes can have tokens inside

lauriii’s picture

Status: Needs review » Needs work

#15.1 and #15.2 should still be checked

lauriii’s picture

Status: Needs work » Needs review

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.

lauriii’s picture

Issue tags: +sprint

LewisNyman’s picture

FileSize
13.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch move_views_classes_from-2329917-26.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll

The last submitted patch, 21: move_views_classes_from-2329917-21.patch, failed testing.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Unless testbot doesn't agree, this looks RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: move_views_classes_from-2329917-26.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
13.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch move_views_classes_from-2329917-30.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: move_views_classes_from-2329917-30.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
13.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,030 pass(es). View

Rerolled and fixed some documentation

lauriii’s picture

FileSize
1.3 KB
14.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,079 pass(es). View

Added documentation to responsive variable

jamesquinton’s picture

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);
  }
Anonymous’s picture

FileSize
19.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,501 pass(es). View

Moved table view classes to twig template file.

Anonymous’s picture

FileSize
19.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,554 pass(es). View

Oops, forgot to remove 2 lines commented out from the old code in the previous patch file. This is the correct one.

Anonymous’s picture

lauriii’s picture

Status: Needs review » Needs work
Issue tags: -frontendunited, -FUDK

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.

Anonymous’s picture

Yes, it would be much better for front-end developers if we are able to stream-line this process. E.g having all class definitions only in the template file and give themers complete control over what go in there and keep the Views UI to pretty much bare-bone. @lauriii, has there been any discussion about this anywhere? Would be nice to follow up on this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
6.24 KB
18.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,284 pass(es). View

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.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

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!

alexpott’s picture

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

lauriii’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
+++ 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.

Anonymous’s picture

I agree that @alexpott suggestion is a more optimised option. But from what I have seen so far, the classes are always defined in one place in the template file. for example:

{%
  set row_classes = [
    row.active ? 'active',
    'custom-class-1',
    'custom-class-2',
  ]
%}

Having to defined the active class differently would - in a way - introduces irregularity in the template structure. Also when the template is overridden, themers might want to add extra classes besides the active class (custom-class-1, custom-class-2, etc...) so (in a way) we are not always necessarily calling the addClass function on empty classes array. It might not be the most optimised, but I believe is the cleanest way.

On the side note, in order to test changes on the views-view-summary-unformatted.html.twig template, you will need to add a contextual filter to the view block with the option Display a summary and show the block on every page. Active class for the view's link will then be checked against the currently viewed url. Took me a while to figure this out so I hope it might be useful for someone else as well.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,720 pass(es), 14 fail(s), and 7 exception(s). View
913 bytes

Fixes #44

Status: Needs review » Needs work

The last submitted patch, 46: move_views_classes_from-2329917-46.patch, failed testing.

Dragan Eror’s picture

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

davidhernandez’s picture

Issue summary: View changes

adding beta evaluation text.

lauriii’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,070 pass(es), 14 fail(s), and 7 exception(s). View
722 bytes

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

Status: Needs review » Needs work

The last submitted patch, 51: move_views_classes_from-2329917-50.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,079 pass(es). View
649 bytes

Ignore latest patch :P

joelpittet’s picture

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

LewisNyman’s picture

Status: Needs review » Needs work

Setting to needs work based on joel's comments

lauriii’s picture

Status: Needs work » Needs review
FileSize
713 bytes
18.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,569 pass(es). View

Yeah that's a good catch

akalata’s picture

Issue tags: -sprint, -Needs issue summary update
FileSize
18.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,996 pass(es), 1 fail(s), and 0 exception(s). View
18.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,994 pass(es), 1 fail(s), and 0 exception(s). View
1.75 KB

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.
akalata’s picture

Status: Needs review » Needs work
Issue tags: +sprint

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

The last submitted patch, 58: 2329917-move_views_classes-58-coding-style.patch, failed testing.

The last submitted patch, 58: 2329917-move_views_classes-58-rerolled.patch, failed testing.

rootwork’s picture

Issue tags: +SprintWeekend2015
lauriii’s picture

Status: Needs work » Needs review
FileSize
18.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,259 pass(es). View
1.43 KB

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

Manuel Garcia’s picture

Status: Needs review » Needs work

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.
davidhernandez’s picture

#64 1 - We might not be able to do anything about this. This looks like the view ui stuff getting integrated. Am I reading that right? 3 - This is ok. The classes will get removed when we get the templates into Classy, but not all classes have to be added through an attribute.

Manuel Garcia’s picture

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.

mortendk’s picture

Issue tags: -sprint +classy

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.

Manuel Garcia’s picture

Status: Needs work » Reviewed & tested by the community

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.

damiankloip’s picture

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

dawehner’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Issue summary: View changes
FileSize
9.76 KB
10.71 KB

@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

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
18.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,258 pass(es). View
1.38 KB

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

davidhernandez’s picture

For #69 - we are not using Twig in strict mode so, yes, Twig does not care.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC based on the interdiff answering #70.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing

@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?

joelpittet’s picture

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

davidhernandez’s picture

#76, the col/row and clearfix classes are showing up correctly for me. I manually tested a view with grid display. The clearfix comes out in the right spots when vertical or horizontal.

I'm leaving needs work for now, since I agree views-view-grid might need attention. The "set classes" are mostly being duplicated in two places, but we can probably streamline it.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
19.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,320 pass(es). View

This needed a reroll after a commit that just happened. The only difference is I fixed some indenting in the views-view-grid template.

Regarding that template, I'm now not sure how to easily refactor it. We're actually dealing with multiple conditions, because of the col/row options from the view, and also the loop index. I'll play with it, but someone else feel free, or maybe we are good the way it is.

davidhernandez’s picture

FileSize
18.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,300 pass(es). View
2.88 KB

I tried refactoring the template, pulling the ifs out and condensing things a bit.

Manuel Garcia’s picture

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?

davidhernandez’s picture

I wouldn't split them. That seems like overkill, and you would have to override both of them when you wanted to make changes in your theme. I think the template could be simplified even further, since it is really just about one series of divs wrapping another, but that is out of scope for this issue. Lets just get the preprocess changes in.

mortendk’s picture

dont split em - thats overkill.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

@davidhernandez - all good points

OK, let's get this one in!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

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

  • alexpott committed 5f05852 on 8.0.x
    Issue #2329917 by lauriii, Manuel Garcia, davidhernandez, akalata,...
davidhernandez’s picture

Rejoice!

mortendk’s picture

so this was banana phase 1 ?
*dance*

Manuel Garcia’s picture

Status: Fixed » Closed (fixed)

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