Remove classes from Views templates

moved templates & cleaned out classes

re roll

Here's a brave new patch -=)

You're right, I got carried away...

+++ b/core/modules/views/templates/views-mini-pager.html.twig
@@ -12,28 +12,28 @@
-    <ul class="pager__items">
...
+    <ul>

pager__items are used by the javascript, please test those bits to ensure things still works. Ajax view ... with a mini pager

Good catch dawehner. I've tested the attached (with stark enabled) using a minipager with ajax, its now working.

I also tested an exposed filter using ajax, it also works.

I also tested an exposed filter using ajax, it also works.

Thank you for manually testing it. It looks fine for me now.

This should fix the failing tests.

@davidhernandez do you think we should include a change to the views classes for js- prefixing in this patch, or tackle that on a different issue?

+++ b/core/modules/views/src/Tests/Handler/AreaEntityTest.php
@@ -158,8 +161,8 @@ public function doTestRender($entities) {
+    $view_class = 'view-dom-id-' . $view->dom_id;
+    $result = $this->xpath('//div[@class = "' . $view_class .'"]/div[1]');

@@ -167,7 +170,8 @@ public function doTestRender($entities) {
+    $view_class = 'view-dom-id-' . $view->dom_id;
+    $result = $this->xpath('//div[@class = "' . $view_class .'"]/div[3]');

Mh, the test now is much less readable ... too bad, anyway, can we open up a follow up to use and for those bits? This would improve a lot the semantics of the code.

Interesting, I thought most tests used Classy. So far we haven't had to do much of this type of thing.

+++ b/core/modules/views/src/Tests/Handler/AreaEntityTest.php
@@ -125,23 +125,26 @@ public function doTestRender($entities) {
+    $header_xpath = '//div[@class = "' . $view_class .'"]/div[1]';
+    $footer_xpath = '//div[@class = "' . $view_class .'"]/div[3]';

@@ -158,8 +161,8 @@ public function doTestRender($entities) {
+    $result = $this->xpath('//div[@class = "' . $view_class .'"]/div[1]');

@@ -167,7 +170,8 @@ public function doTestRender($entities) {
+    $result = $this->xpath('//div[@class = "' . $view_class .'"]/div[3]');

+++ b/core/modules/views/src/Tests/Handler/AreaViewTest.php
@@ -44,7 +44,7 @@ public function testViewArea() {
+    $this->assertTrue(strpos($output, 'view-dom-id-'. $view->dom_id) !== FALSE, 'The test view is correctly embedded.');

Minor: concatenating should always have spaces around the dot per https://www.drupal.org/coding-standards#concat.

@dawehner Yeah i couldn't think of a way to make the test easier to read :S

@Cottser thanks! Fixing that on this patch now.

@davidhernandez I agree about the tests. Not sure I follow, but in my opinion we should tackle switching to js- css classes on a separate issue.

@manuel go with the js-prefix so we get it fixed right away :)
is what our documentation is also saying

Alright, the attached patch includes the following:
Replaced all instances of:

• view-dom-id- with js-view-dom-id-
• pager__items with js-pager__items

I did not tackle the classes on views-view-table.html.twig (responsive-enabled and sticky-enabled) because these are not views specific, but used throughout core, so perhaps we should change those on a separate issue.

+++ b/core/modules/system/templates/pager.html.twig
@@ -34,7 +34,7 @@
-    <ul class="pager__items">
+    <ul class="js-pager__items">

So this is super confusing, honestly. For me we basically loose quite some of the semanticness of those css classes? Maybe this is simply something we don't care about?

I think we should especially add the js- prefix to the dom-id classes, because frankly I'm sick of explaining to people what they are for ;P

Rerolling because #2187113: Incorrect usage of attributes in twig templates resulting in possible duplicate attributes. got in.

@davidhernandez thanks for the explanation, I will tackle that next (.pager__items)

I agree with Cottser that we should add the js- prefix to the dom-id classes... it makes a lot of sense to me to add the prefix because they are only used for javascript.

Here it is without removing pager__items from the system pager nor classy, and adding js- prefix to all.

I manually tested that ajax enabled views minipager, normal pager and exposed filters work.

I am now getting failures on AggregatorRenderingTest and PagerTest, but I don't understand why. The class being tested for is still in the core template, we're just adding a new one... let's see if the bot finds the same, if it does I could use some help figuring this out.

OK, learning a bit of xpath madness along the way. Hopefully this one will come back green.

removing classes from :
views-view-grid.html.twig
views-view-summary.html.twig
views-view-summary-unformatted.html.twig

@davidhernandez have we need to remove classes from classy's view templates also ? For now i have removed only from /core/modules/views/templates/. Please verify.

forget to trigger test bot :)

ok !! then patch would be fine i guess :)

1. +++ b/core/modules/views/templates/views-view-grid.html.twig
   @@ -28,16 +28,12 @@
   -    'views-view-grid',
   ...
   -      'views-row',
   
   @@ -45,7 +41,6 @@
   -      'views-col',
   

   We should manually verify (using stark theme) that the grid actually works as a grid without these classes. To me it would make no sense to remove these if the grid style stops working without them.

2. +++ b/core/modules/views/templates/views-view-grid.html.twig
   @@ -28,16 +28,12 @@
        options.alignment,
   -    'cols-' ~ options.columns,
   -    'clearfix',
      ]
   

   This would break the alignment options functionality, again, we should manually test this using stark.

3. +++ b/core/modules/views/templates/views-view-summary-unformatted.html.twig
   @@ -21,7 +21,7 @@
   -  {{ options.inline ? '<span' : '<div' }} class="views-summary views-summary-unformatted">
   +  {{ options.inline ? '<span' : '<div' }} >
   
   +++ b/core/themes/classy/templates/views/views-view-summary.html.twig
   @@ -18,8 +18,8 @@
   -<div class="item-list">
   -  <ul class="views-summary">
   +<div>
   +  <ul>
   

   Yes, totally missed those earlier.. thanks!

Manually tested this and alignment does break in the stark theme. See the screenshot of stark theme - admin people view in grid view with and without classes.

Updated the patch

Thanks @saki007ster for the testing and the updated patch.

On the interdiff:

+++ b/core/modules/views/templates/views-view-summary-unformatted.html.twig
@@ -21,7 +21,7 @@
-  {{ options.inline ? '<span' : '<div' }} >
+  {{ options.inline ? '<span' : '<div' }} class="views-summary views-summary-unformatted">

+++ b/core/modules/views/templates/views-view-summary.html.twig
@@ -20,8 +20,8 @@
-<div>
-  <ul>
+<div class="item-list">
+  <ul class="views-summary">

These should be ok to be removed in my opinion.

@Manuel Garcia updated the patch and interdiff.

Thank you @saki007ster - I have fixed just a tiny missing space for coding standards, and reviewed all other templates on core/modules/views/templates/ directory, nothing else to do that I can see. I'd say this patch is good to go!

I'm reviewing this patch

Added screenshots for grid, pager item

formatted list screenshot

screenshots for more link, unformatted, table display. these seem fine. I'm done adding screenshots.

+++ b/core/modules/system/templates/pager.html.twig
@@ -34,7 +34,7 @@
-    <ul class="pager__items">
+    <ul class="pager__items js-pager__items">

It seems alright now even it is a bit sad to have both classes available.

thanks @tombo for the screenshots!
@dawehner thnx for rtbcing! - the reason to have both classes is to make it simpler to remove all styling form the core template, keeping the js functionality (see comment #29).

1. +++ b/core/modules/views/tests/modules/views_test_data/templates/views-view--frontpage.html.twig
   @@ -11,7 +11,7 @@
     *     - view-[css_name]
     *     - view-id-[view_name]
     *     - view-display-id-[display_name]
   - *     - view-dom-id-[dom_id]
   + *     - js-view-dom-id-[dom_id]
   

   Are we sure that this documentation is correct? Afaics this will now have no classes at all. I know this is not introduced by this patch but we are changing a line here.

2. +++ b/core/themes/classy/templates/views/views-view-summary.html.twig
   @@ -18,8 +18,8 @@
   -<div class="item-list">
   -  <ul class="views-summary">
   +<div>
   +  <ul>
   
   +++ b/core/themes/classy/templates/views/views-view.html.twig
   @@ -36,7 +36,7 @@
   -    dom_id ? 'view-dom-id-' ~ dom_id,
   

   Why are we removing classes from classy?

Thanks @alexpott for the input,

About 1. - Good point, I've updated the documentation on that to be a little more generic, since not all classes are for styling purposes now. If anyone has a better idea on what to say here let me know.

About 2. -

+++ b/core/themes/classy/templates/views/views-view-summary.html.twig
@@ -18,8 +18,8 @@
-<div class="item-list">
-  <ul class="views-summary">
+<div>
+  <ul>

Good catch, that change got introduced on #39 and only @davidhernandez noticed... oops! I've moved that out of this patch now.

+++ b/core/themes/classy/templates/views/views-view.html.twig
@@ -36,7 +36,7 @@
-    dom_id ? 'view-dom-id-' ~ dom_id,

We're actually changing that for the new js- prefixed one, so not a removal ;)

Changes in #58 looks fine.

Committed 1827c1e and pushed to 8.0.x. Thanks!

+++ b/core/modules/views/tests/modules/views_test_data/templates/views-view--frontpage.html.twig
@@ -5,13 +5,12 @@
  * - attributes: Remaining HTML attributes for the element including:
- *   - class: HTML classes that can be used to style contextually
- *     through CSS, including:
+ *   - class: HTML classes including:
  *     - view
  *     - view-[css_name]
  *     - view-id-[view_name]
  *     - view-display-id-[display_name]
- *     - view-dom-id-[dom_id]
+ *     - js-view-dom-id-[dom_id]

I think we should just copy the text from views-view.html.twig. ie attributes: Remaining HTML attributes for the element. And considering this is test code I think we should fix it here. Fixed on commit.

Cheers @alexpott!