Problem/Motivation

#2407735: Remove classes from system templates m*.html.twig removed the 'menu' classes from menu.html.twig. The toolbar module unfortunately relies on these classes currently in both its CSS and JS.

Proposed resolution

Add the classes back until #1944572: Remove "ul.menu" dependency to prevent theme clashes is resolved. Before/after screenshots from Stark:

Before

After

Remaining tasks

  • Patch
  • Review
  • Commit

User interface changes

Restore toolbar appearance and functionality in any theme that doesn't extend from Classy.

API changes

n/a

CommentFileSizeAuthor
#2 2447829-1-after.png26.18 KBstar-szr
#2 2447829-1-before.png33.4 KBstar-szr
#1 2447829-1.patch543 bytesstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2447829-1.patch543 bytes
star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes
StatusFileSize
new 2447829-1-before.png33.4 KB
new 2447829-1-after.png26.18 KB

Screenshots for the issue summary.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes
star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes
lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

hass’s picture

hass commented

Does this make #1944572: Remove "ul.menu" dependency to prevent theme clashes critical?

mortendk’s picture

mortendk commented

what - are we now adding in classes *gasp* :(
imho we should fix the real problem instead & get #1944572: Remove "ul.menu" dependency to prevent theme clashes in
@hass yes it kinda do :/

hass’s picture

hass commented

What are you talking about? We are fixing the real issue.

mortendk’s picture

mortendk commented

are were adding in a class to core now just to to remove it again when #1944572 gets in ?

star-szr’s picture

star-szr
he/him
Primary language English
commented

There is currently coupling of .menu classes with the toolbar, as long as that exists the classes should be there in every theme. The fact that the coupling exists (#1944572: Remove "ul.menu" dependency to prevent theme clashes) is not release blocking at all IMO. Let's fix the regression and move forward please.

mortendk’s picture

mortendk commented

@cottser check was just asking also why i didnt change status & is breathing in a paperbag ;)

star-szr’s picture

star-szr
he/him
Primary language English
commented
Title: Add some classes back to menu.html.twig for toolbar functionality » Add "menu" classes back to menu.html.twig for toolbar functionality

:D

hass’s picture

hass commented

We need to get #1944572: Remove "ul.menu" dependency to prevent theme clashes done before final D8. This is a critical bugfix for contrib themes. Otherwise we may discuss why we cannot change theme templates post D8 release. :-(

alexpott’s picture

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

I agree with @Cottser - #1944572: Remove "ul.menu" dependency to prevent theme clashes is not release blocking. Committed 1506a99 and pushed to 8.0.x. Thanks!

  • alexpott committed 1506a99 on 8.0.x 
    Issue #2447829 by Cottser: Add "menu" classes back to menu.html.twig for...

Status: Fixed » Closed (fixed)

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