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

Files: 
CommentFileSizeAuthor
#26 interdiff-mtempaltes-26-26.txt885 bytesmortendk
#26 templates-cleanup-m-26.diff3.28 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,912 pass(es). View
#24 m-interdiff.txt331 bytesmortendk
#24 m-templates-no-ragrets.png1023.07 KBmortendk
#24 templates-cleanup-m-24.diff3.11 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,821 pass(es). View
#22 templates-cleanup-m-22.diff1.71 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,890 pass(es). View
#4 templates-cleanup-m.diff3.59 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,754 pass(es). View
#1 issue-2407735-1.patch10.87 KBSivaji
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,892 pass(es). View

Comments

Sivaji’s picture

Status: Active » Needs review
FileSize
10.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,892 pass(es). View
davidhernandez’s picture

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 Classy theme for updates. Thanks.

davidhernandez’s picture

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

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,754 pass(es). View
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue tags: +drupalcafe-feb2015
Xen’s picture

Assigned: Unassigned » Xen
Xen’s picture

Status: Needs review » Reviewed & tested by the community

Looked through all files, and they render as intended.

Xen’s picture

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

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

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...
Cottser’s picture

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

we want a followup ;)

davidhernandez’s picture

I uploaded a patch in the issue Cottser linked to.

alexpott’s picture

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

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

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,890 pass(es). View

Removing maintenance template from the patch.

mortendk’s picture

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

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -drupalcafe-feb2015
FileSize
3.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,821 pass(es). View
1023.07 KB
331 bytes

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

interdiff added

no visual regressions on installer

Manuel Garcia’s picture

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

Status: Needs work » Needs review
FileSize
3.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,912 pass(es). View
885 bytes

comments removed intediff added

Manuel Garcia’s picture

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

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

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

hass’s picture

Ahh classy kept the menu class.

Cottser’s picture

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