Problem/Motivation

This child issue will tackle most of the Views module templates. views-view.html.twig is handled in #2510794: Remove unnecessary markup from views-view.html.twig

After the removal of CSS classes from the core templates, many unnecessary HTML tags were left behind. They were not removed in the process of removing classes and other work, because removing them requires paying particular attention to any affect on the output.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal because nothing is broken.
Unfrozen changes Unfrozen because it only changes templates and possibly CSS which are both unfrozen.
Prioritized changes The main goal of this issue is to improve themer experience.
Disruption There should be minimal disruption.

Proposed resolution

Go through each core template and remove any unnecessary markup. This will require looking at the output to see if things shift around in unwanted ways. Various CSS files may also need checking. For example, if there is CSS like .some_class > div > .some_other_class the CSS should be adjusted, or deleted.

Evaluation criteria:

  • divs and spans with no additional attributes should be removed mercilessly unless they justify their existence.
  • Semantic tags (footer, article, etc.) should be retained even if they have no attributes.

User interface changes

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

davidhernandez’s picture

Status: Active » Needs review
FileSize
2.78 KB

lol I obliterated everything in the views-view template.

Status: Needs review » Needs work

The last submitted patch, 2: remove_unnecessary-2497455-2.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review

this looks perfect +1 RTBC as soon as testbot is done complaining over us killing divitis

dawehner’s picture

Seems pretty neat!

dawehner’s picture

Well, some of the test coverage is assuming some specific divs, like this:

    $view_class = 'js-view-dom-id-' . $view->dom_id;
    $header_xpath = '//div[@class = "' . $view_class . '"]/div[1]';
    $footer_xpath = '//div[@class = "' . $view_class . '"]/div[3]';

so things are a bit tricky to get done properly. You need to be able to select them, especially because you need similar kind of functionality in javascript.

mortendk’s picture

hmm should it not test on classy's markup & classes and not core's ?

davidhernandez’s picture

Looking at Drupal\views\Tests\Plugin\StyleUnformattedTest, it is not a web test. It is a views unit test ... which is checking resultant markup. :(

We don't want to tell it to use Classy (and we want to avoid that in general) so the test will need to be reworked. That looks fixable, but I didn't look at the other fails.

star-szr’s picture

Status: Needs review » Needs work

@dawehner can you expand on this a bit please? Would this be for ajax functionality or something?

You need to be able to select them, especially because you need similar kind of functionality in javascript.

star-szr’s picture

Issue tags: +VDC
Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Having a go at this

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
4.04 KB
1.26 KB

This should fix some of the failing tests, specifically:

  • StyleMappingTest
  • StyleUnformattedTest

AreaEntityTest will need some more effort with the xpaths... any takers?

Status: Needs review » Needs work

The last submitted patch, 12: remove_unnecessary-2497455-12.patch, failed testing.

Manuel Garcia’s picture

I agree with @dawehner that removing all divs from views-view.html.twig might be a bit too much.

What he is talking about is probably the js for ajax pagination and exposed filters.

So we should test this with stark, on a view with ajax enabeled pager, exposed filters, etc. which couldbe broken with this patch atm.

Actually I just tried doing that, but there is this bug which breaks ajax pagination atm #2314185: [PP-1] DrupalBehaviorError: attach ; quickedit: entityElement.get(...) is undefined eek! so make sure to disable quickedit I suppose.

star-szr’s picture

If that's the case it sounds like we might at this stage be missing some js- prefixes.

dawehner’s picture

What he is talking about is probably the js for ajax pagination and exposed filters.

Yeah, should we not add usage of <header> and <footer> instead?
But yeah most template changes are totally fine. Its just the views-view one, which is kinda odd. Just imagine it as a mini version of page.html.twig

davidhernandez’s picture

What was removed from views-view were just empty divs and 'if's which were there because of the divs, so I wouldn't call it odd. It actually makes it a lot easier to read now. But I agree it may lack some structure, if structure is desired. (I didn't look at all at the resultant markup) We could explore using header, footer, etc, but my concern would be in deviating too far from the sister template in Classy.

star-szr’s picture

Yeah, I think if we go with header, footer, etc. then we should update Classy as well.

For the sake of this issue's scope how about we split handling views-view.html.twig into its own issue to discuss further since it's special?

dawehner’s picture

For the sake of this issue's scope how about we split handling views-view.html.twig into its own issue to discuss further since it's special?

Great idea!

davidhernandez’s picture

Split off the divitis, too? (Scope-wise, I wasn't proposing we touch any Classy templates here, so I agree on that point.) I think we could remove the divs here and create a follow up to address the structure of the template along with the Classy template.

star-szr’s picture

What I am proposing is to remove all views-view template related changes from the patch, and still curb divitis in the other views templates (sounds like maybe it could use some manual testing too). Then a separate (not really follow up) issue to handle the beast.

davidhernandez’s picture

Outside of the views-view change this is like a 10 line patch, so I'm not sure what we gain by opening another issue. :/

Manuel Garcia’s picture

Hummm actually, I just tested the latest patch, using stark, with an ajax view, both the exposed filters work, as well as the pagination, so we've done a better job than we thought apparently =)

Mind you you have to disable quickedit but that has nothing to do with this issue.

So I suppose its a matter of fixing the failing tests...
Drupal\views\Tests\Handler\AreaEntityTest 82 passes 10 fails 1 exceptions

emma.maria’s picture

The last submitted patch, 12: remove_unnecessary-2497455-12.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
6.35 KB
2.31 KB

Here's a go at fixing some of the faling tests. Stuck with the failing ones though.

Status: Needs review » Needs work

The last submitted patch, 27: remove_unnecessary-2497455-27.patch, failed testing.

star-szr’s picture

To respond to #22, the reason I wanted to split off the views-view template is that these issues (divitis removal) were supposed to be straightforward other than the manual testing. IMO it is no longer straightforward if we need to discuss (for example) what semantic tags to add to views-view.html.twig. So I know it would be a small patch without the views-view changes but we could get it done and then worry about the big fish in another issue :)

davidhernandez’s picture

Alright. I'll make another issue.

davidhernandez’s picture

davidhernandez’s picture

Status: Needs work » Needs review
mortendk’s picture

Status: Needs review » Reviewed & tested by the community

removed the divs loads of happyness ;)
+1 on the followup issue btw

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Markup is not yet frozen. Committed d4f97bb and pushed to 8.0.x. Thanks!

  • alexpott committed d4f97bb on 8.0.x
    Issue #2497455 by Manuel Garcia, davidhernandez, Cottser, dawehner:...

Status: Fixed » Closed (fixed)

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