See parent issue(s) and related issue(s) for details,

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

cleanup up

  • core/modules/system/templates/maintenance-page.html.twig
  • core/modules/system/templates/mark.html.twig
  • core/modules/system/templates/menu-local-action.html.twig
  • core/modules/system/templates/menu-local-task.html.twig
  • core/modules/system/templates/menu-local-tasks.html.twig
  • core/modules/system/templates/menu.html.twig

Remaining tasks

User interface changes

API changes

Original report by @username

CommentFileSizeAuthor
#26 interdiff-mtempaltes-26-26.txt885 bytesmortendk
#26 templates-cleanup-m-26.diff3.28 KBmortendk
#24 m-interdiff.txt331 bytesmortendk
#24 m-templates-no-ragrets.png1023.07 KBmortendk
#24 templates-cleanup-m-24.diff3.11 KBmortendk
#22 templates-cleanup-m-22.diff1.71 KBhussainweb
#4 templates-cleanup-m.diff3.59 KBmortendk
#1 issue-2407735-1.patch10.87 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-2407735-1.patch10.87 KB
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs review » 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 m*.html.twig to Classy » Remove classes from system templates m*.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.

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
templates-cleanup-m.diff3.59 KB
mortendk’s picture

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

mortendk CreditAttribution: mortendk commented
Issue tags: +drupalcafe-feb2015
Xen’s picture

Xen CreditAttribution: Xen commented
Assigned: Unassigned » Xen
Xen’s picture

Xen CreditAttribution: Xen at Reload commented
Status: Needs review » Reviewed & tested by the community

Looked through all files, and they render as intended.

Xen’s picture

Xen CreditAttribution: Xen commented
Assigned: Xen » Unassigned

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: templates-cleanup-m.diff, failed testing.

Status: Needs work » Needs review

mortendk queued 4: templates-cleanup-m.diff for re-testing.

mortendk queued 4: templates-cleanup-m.diff for re-testing.

mortendk’s picture

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

looks like testboot was drunk setting this back to RTBC after tester from xen

alexpott’s picture

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

Templates are not frozen in beta. Committed 2b9b727 and pushed to 8.0.x. Thanks!

  • alexpott committed 2b9b727 on 8.0.x 
    Issue #2407735 by mortendk, sivaji@knackforge.com: Remove classes from...
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

This caused a visual regression in Seven's installer. It looks like the original maintenance-task-list.html.twig should have been copied to Classy. Follow-up created unless we want to revert: #2426589: Visual regression: Missing 'task-list' class in Seven installer

mortendk’s picture

mortendk CreditAttribution: mortendk commented

we want a followup ;)

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

I uploaded a patch in the issue Cottser linked to.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Fixed » Needs work

We should not have been changing maintenance-task-list.html.twig installer theming is not at all like regular theming. Revert this patch.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Alex, did you push that revert? I'm not picking it up.

  • alexpott committed f671f3c on 8.0.x 
    Revert "Issue #2407735 by mortendk, sivaji@knackforge.com: Remove...
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
templates-cleanup-m-22.diff1.71 KB

Removing maintenance template from the patch.

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Status: Needs review » Needs work

maintenance-page.html.twig should still be cleaned up, whats beeing reverted is the maintenance-task-list.html.twig
Which we should rename in a followup issue to install-tast-list.html.twig or something like that to make sense

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -drupalcafe-feb2015
FileSize
templates-cleanup-m-24.diff3.11 KB
m-templates-no-ragrets.png1023.07 KB
m-interdiff.txt331 bytes

maintenance-task-list.html.twig is pulled of the patch

interdiff added

no visual regressions on installer

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia commented
Status: Needs review » Needs work 
+++ b/core/modules/system/templates/maintenance-page.html.twig
@@ -21,15 +21,15 @@
       </div>{# /.name-and-slogan #}

@@ -47,13 +47,13 @@
     </aside>{# /.layout-sidebar-first #}
...
     </aside>{# /.layout-sidebar-second #}

We should remove these comments since they refer to the classes we are removing.

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Status: Needs work » Needs review
FileSize
templates-cleanup-m-26.diff3.28 KB
interdiff-mtempaltes-26-26.txt885 bytes

comments removed intediff added

Manuel Garcia’s picture

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

Alright this now good to go. Double checked that all removed classes are still in classy, so we are safe when it comes to visual regressions.

alexpott’s picture

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

Templates are not frozen in beta. Committed bc49133 and pushed to 8.0.x. Thanks!

  • alexpott committed bc49133 on 8.0.x 
    Issue #2407735 by mortendk, sivaji@knackforge.com, hussainweb: Remove...

Status: Fixed » Closed (fixed)

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

hass’s picture

hass CreditAttribution: hass commented

How has this change not broken the toolbar that require .menu class?

hass’s picture

hass CreditAttribution: hass commented

Ahh classy kept the menu class.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

That's a good catch @hass, if you switch your theme to Stark and navigate to the homepage you get a kinda broken toolbar. Not really this issue's fault, at this point I agree that toolbar shouldn't be using such a generic class name for both its CSS and especially not its JS. So it seems we need to add some classes back until #1944572: Remove "ul.menu" dependency to prevent theme clashes is resolved:

#2447829: Add "menu" classes back to menu.html.twig for toolbar functionality