Olivero sets inconsitent classes between its menu and book tree templates for active trail.
templates/navigation/menu.html.twig: * - in_active_trail: TRUE if the link is in the active trail.
templates/navigation/menu.html.twig: item.in_active_trail ? 'menu__item--active-trail',
templates/navigation/menu.html.twig: item.in_active_trail ? 'menu__link--active-trail',
templates/navigation/book-tree.html.twig: item.in_active_trail ? 'menu-item--active-trail',
templates/navigation/book-tree.html.twig: item.in_active_trail ? 'menu__link--active-trail',
Make both classes consistent.
Either do so in a way that doesn't require test changes when Olivero is the default theme, or if not, adjust the test coverage here.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 3276618-8.patch | 3.09 KB | eojthebrave |
| #5 | 3276618-5.patch | 872 bytes | mherchel |
| #2 | 3276618.patch | 638 bytes | catch |
Comments
Comment #2
catchThis just makes it the same as the book one, which would mean no test changes or Olivero special casing in the menu active trail test.
We might want a follow-up to continue the work in #3248309: AssertBreadcrumbTrait should not rely on Classy so that MenuActiveTrailTrait doesn't rely on the standard profile, which would allow Olivero to diverge without breaking any tests.
If we don't do this here, then I think #3219959: Update standard profile so Olivero is the default theme should just change the assertion in that trait without special casing Olivero (as in, just switch from one to the other), but in that case, I think this issue ought to bring the book classes into line.
Comment #3
catchOpened #3276652: AssertMenuActiveTrailTrait should not rely on classy/bartik for the follow-up.
Comment #4
mherchelThe patch in #2 works, but
.menu-itemis not a proper CSS BEM block. We should remove that instead and standardize on the.menuCSS BEM blockNew patch incoming shortly
Comment #5
mherchelComment #6
catchI thought you might say that, this is fine, but it means we need to resolve #3276652: AssertMenuActiveTrailTrait should not rely on classy/bartik, or at least make the trait hard code Olivero markup instead of Bartik/classy markup, before Olivero can be stable then, to avoid test failures.
Comment #7
gábor hojtsyComment #8
eojthebraveHere's a version of the patch from #5 that updates AssertMenuActiveTrailTrait with an or statement so that it'll work with either the CSS class from classy. This is at least a stop-gap measure to work around this issue until we can come up with a better long term solution in #3276652: AssertMenuActiveTrailTrait should not rely on classy/bartik
Comment #9
catchOK I think that is reasonable, basically we're hard-coding two themes instead of one, but it means contrib tests that rely on classy won't be broken (which they would be if we just changed the assertions to match Olivero), and it's not obvious how to change this test to be stark-compatible without changing the assertions a lot. #3276652: AssertMenuActiveTrailTrait should not rely on classy/bartik is already open for that.
Comment #10
gábor hojtsyComment #13
gábor hojtsyLooks good. We discussed this in person, and this was the only sensible backwards compatible way to approach this.