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
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
Comment | File | Size | Author |
---|---|---|---|
#30 | remove_unnecessary-2497455-30.patch | 1.41 KB | davidhernandez |
#27 | interdiff.txt | 2.31 KB | Manuel Garcia |
#27 | remove_unnecessary-2497455-27.patch | 6.35 KB | Manuel Garcia |
#12 | interdiff.txt | 1.26 KB | Manuel Garcia |
#12 | remove_unnecessary-2497455-12.patch | 4.04 KB | Manuel Garcia |
Comments
Comment #1
davidhernandezComment #2
davidhernandezlol I obliterated everything in the views-view template.
Comment #4
mortendk CreditAttribution: mortendk as a volunteer commentedthis looks perfect +1 RTBC as soon as testbot is done complaining over us killing divitis
Comment #5
dawehnerSeems pretty neat!
Comment #6
dawehnerWell, some of the test coverage is assuming some specific divs, like this:
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.
Comment #7
mortendk CreditAttribution: mortendk as a volunteer commentedhmm should it not test on classy's markup & classes and not core's ?
Comment #8
davidhernandezLooking 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.
Comment #9
star-szr@dawehner can you expand on this a bit please? Would this be for ajax functionality or something?
Comment #10
star-szrComment #11
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedHaving a go at this
Comment #12
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedThis should fix some of the failing tests, specifically:
AreaEntityTest will need some more effort with the xpaths... any takers?
Comment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedI 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.
Comment #15
star-szrIf that's the case it sounds like we might at this stage be missing some js- prefixes.
Comment #16
dawehnerYeah, 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
Comment #17
davidhernandezWhat 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.
Comment #18
star-szrYeah, 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?
Comment #19
dawehnerGreat idea!
Comment #20
davidhernandezSplit 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.
Comment #21
star-szrWhat 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.
Comment #22
davidhernandezOutside of the views-view change this is like a 10 line patch, so I'm not sure what we gain by opening another issue. :/
Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedHummm 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
Comment #24
emma.mariaComment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedHere's a go at fixing some of the faling tests. Stuck with the failing ones though.
Comment #29
star-szrTo 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 :)
Comment #30
davidhernandezAlright. I'll make another issue.
Comment #31
davidhernandezCreated the other. #2510794: Remove unnecessary markup from views-view.html.twig
Comment #32
davidhernandezComment #33
mortendk CreditAttribution: mortendk as a volunteer commentedremoved the divs loads of happyness ;)
+1 on the followup issue btw
Comment #34
alexpottMarkup is not yet frozen. Committed d4f97bb and pushed to 8.0.x. Thanks!