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

CommentFileSizeAuthor
#14 remove_classes_from-2407715-14.patch2.7 KBmortendk
#11 remove_classes_from_2407715_11.patch7.84 KBManjit.Singh
#9 remove_classes_from-2407715-9.patch2.7 KBdavidhernandez
#5 issue-2407715-5.patch4.93 KBdernetzjaeger
#2 interdiff.txt5.4 KBseantwalsh
#2 2407715-2.patch3.28 KBseantwalsh
#1 issue-2407715-1.patch6.66 KBSivaji_Ganesh_Jojodae
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commented
Status: Active » Needs review
FileSize
issue-2407715-1.patch6.66 KB
seantwalsh’s picture

seantwalsh
he/him
Primary language English
Location Eatontown, NJ
CreditAttribution: seantwalsh commented
FileSize
2407715-2.patch3.28 KB
interdiff.txt5.4 KB

Remove "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.

Status: Needs review » Needs work

The last submitted patch, 2: 2407715-2.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

visually-hidden should stay. It is functional.

The test is failing because you removed the attribute from block--system-menu-block.html.twig.

+++ b/core/modules/system/templates/block--system-menu-block.html.twig
@@ -40,16 +40,8 @@
-<nav{{ attributes.addClass(classes) }} role="navigation" aria-labelledby="{{ heading_id }}">

The addClass() should be removed, but leave the attribute.

dernetzjaeger’s picture

dernetzjaeger CreditAttribution: dernetzjaeger commented
Status: Needs work » Needs review
FileSize
issue-2407715-5.patch4.93 KB

Status: Needs review » Needs work

The last submitted patch, 5: issue-2407715-5.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs work » Postponed

Postponing 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.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Title: Copy system templates b*.html.twig to Classy » Remove classes from system templates b*.html.twig
Issue summary: View changes
Status: Postponed » Needs work

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs work » Needs review
FileSize
remove_classes_from-2407715-9.patch2.7 KB

It 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.

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Status: Needs review » Reviewed & tested by the community

the 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 issue

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh commented
FileSize
remove_classes_from_2407715_11.patch7.84 KB

Add all changes in one file that we have discussed above.

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Issue summary: View changes

@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

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Issue summary: View changes
mortendk’s picture

mortendk CreditAttribution: mortendk commented
FileSize
remove_classes_from-2407715-14.patch2.7 KB

reuploads davids patch from #9 & updated the issue, to reflect the changes after the move of templates into classy

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Template changes are permitted during beta. Committed af53fb6 and pushed to 8.0.x. Thanks!

  • alexpott committed af53fb6 on 8.0.x 
    Issue #2407715 by crowdcg, davidhernandez, mortendk, sivaji@knackforge....

Status: Fixed » Closed (fixed)

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