Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Remove non essential classes from core - the classes are in classy, cores templates should be as clean as possible.
js required classes should be prefixed to js- according to code standards
simpletest should be done with classy theme, not stark
See parent issue(s) and related issue(s) for details,
Proposed resolution
Twig Templates to cleanup
- core/modules/system/templates/image.html.twig
- core/modules/system/templates/indentation.html.twig
- core/modules/system/templates/input.html.twig
core/modules/system/templates/install-page.html.twiginstall-page was not copied to Classy. Do not modify it.- core/modules/system/templates/item-list.html.twig
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | image-before.png | 308.19 KB | rteijeiro |
| #21 | image-after.png | 308.19 KB | rteijeiro |
| #20 | cleanup-templates-i-3.diff | 1.72 KB | mortendk |
| #19 | cleanup-templates-i-2.diff | 873 bytes | mortendk |
| #7 | cleanup-templates-i.diff | 1.44 KB | mortendk |
Comments
Comment #1
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #2
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #3
davidhernandezPostponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme for updates. Thanks.
Comment #4
davidhernandezUn-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.
Comment #5
davidhernandezThe "set classes" is not indented correctly in the template. I wonder if we can fix that in this issue. It is a minor thing.
Comment #6
mortendk CreditAttribution: mortendk commentedComment #7
mortendk CreditAttribution: mortendk commentedComment #8
mortendk CreditAttribution: mortendk commentedComment #11
mortendk CreditAttribution: mortendk commentedComment #12
kasperg CreditAttribution: kasperg commentedI'll take a stab at reviewing this.
Comment #13
kasperg CreditAttribution: kasperg commentedI have reviewed and tested the patch and it works as expected.
Classes are removed from item lists and images when using image styles.
I found changes to indentation.html.twig hard to test since theme_indentation is used by default. Copying the contents of the template to classys indentation.html.twig and rendering the admin menu tree for classy shows that it works.
Comment #14
davidhernandezI think the class and blank space shouldn't be removed from the indentation template. That is essentially its whole purpose. Without it you are running through a 'for' loop that just prints an empty div.
Comment #15
mortendk CreditAttribution: mortendk commentedprinting a
&nsp;is not very 2015 its more like 2001.We remove any class that is not essential to the functionality.
An indent is a visual enhancement, thats not essential for the module / template to work
Comment #16
mortendk CreditAttribution: mortendk commentedComment #17
mortendk CreditAttribution: mortendk commentedi cant set my own stuff to rtbc *doh*
Comment #18
davidhernandezI think the indentation class is essential to the templates functionality. Otherwise, what is it doing? You have a 'for' loop printing a series of empty divs. And in the land of dream markup, the divs would get removed, so then it isn't doing anything. It's a very 2001 thing, but that seems to be its purpose.
Comment #19
mortendk CreditAttribution: mortendk commented@david you correct, actually we should not even have the template in classy, im creating a followup issue for this, as well as a prober naming for the .indentation class as it shold be .js-indentation
Comment #20
mortendk CreditAttribution: mortendk commentedon the other hand lets just kill off indentation.html.twig right away
Comment #21
rteijeiro CreditAttribution: rteijeiro commentedGood! Well done @mortendk!!
Image BEFORE
Image AFTER
Comment #22
alexpottSo copying indentation.html.twig to classy was wrong since now it is being used - contrary to the comments. The correct thing to do here is to remove the it from classy. And not worry about the one in system since it is not in used - as per the comment.
Comment #23
davidhernandezIsn't that what the patch in #20 does? Removes it for Classy.
Comment #24
mortendk CreditAttribution: mortendk commented@alex the fil got removed from classy in #20 - setting it back to rtbc
Comment #25
alexpottTemplates are not frozen in beta. Committed a00b10d and pushed to 8.0.x. Thanks!