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
Proposed resolution
Templates to modfy
- core/modules/system/templates/block--system-branding-block.html.twig
- core/modules/system/templates/block--system-menu-block.html.twig
- core/modules/system/templates/breadcrumb.html.twig
Test that no classes are clashing with js functionality, if so prefix classes with js- so its clear that the class is required by js.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#14 | remove_classes_from-2407715-14.patch | 2.7 KB | mortendk |
#11 | remove_classes_from_2407715_11.patch | 7.84 KB | Manjit.Singh |
#9 | remove_classes_from-2407715-9.patch | 2.7 KB | davidhernandez |
#5 | issue-2407715-5.patch | 4.93 KB | dernetzjaeger |
#2 | interdiff.txt | 5.4 KB | seantwalsh |
Comments
Comment #1
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #2
seantwalshRemove "set classes" and {{ attributes.addClass(classes) }} from core/modules/system/templates/block--system-menu-block.html.twig
Wondering if class="vissually-hidden" should also be removed. There is CSS for these in system so left them in for now.
Comment #4
davidhernandezvisually-hidden should stay. It is functional.
The test is failing because you removed the attribute from block--system-menu-block.html.twig.
The addClass() should be removed, but leave the attribute.
Comment #5
dernetzjaeger CreditAttribution: dernetzjaeger commentedComment #7
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 #8
davidhernandezUn-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.
Comment #9
davidhernandezIt looks like the system menu still picks up an ID attribute from somewhere but I'm not sure where it's coming from. We may need to track that down.
Comment #10
mortendk CreditAttribution: mortendk commentedthe classes
site-slogan, site.name, site-logo
are only used in bartik.block-menu
only in bartik.breadcrumb
is in system.theme.css - i would suggest we collect to a css followup and move specific theme elements to seven & bartik, but that is out of scope of this issueComment #11
Manjit.SinghAdd all changes in one file that we have discussed above.
Comment #12
mortendk CreditAttribution: mortendk commented@11 template files in classy have been reorganized & have been copied over, your patch duplicate the templates in classy.
i will update the issue summary to reflect that.
patch #9 is rtbc
Comment #13
mortendk CreditAttribution: mortendk commentedComment #14
mortendk CreditAttribution: mortendk commentedreuploads davids patch from #9 & updated the issue, to reflect the changes after the move of templates into classy
Comment #15
alexpottTemplate changes are permitted during beta. Committed af53fb6 and pushed to 8.0.x. Thanks!