Rewrite the menu CSS components inline with our CSS standards

You know what would be really helpful here... .menu-item--leaf class. But that may be out of scope of this issue. Is there another issue to rename this class @mortendk?

I double checked, there is no other uses of the 'leaf' class than li elements in core but it is very generic a class name needs some BEM!

This is an example of an issue were we need to this about the entire component, because if we weaken the selector in Seven we introduce potential for regressions because the module CSS is now stronger, if we change the module CSS we should also look at Bartik. We can use #1995272: [Meta] Refactor module CSS files inline with our CSS standards to add issues that affect all themes, so we can tackle it everywhere in one go.

Ok hopefully this is on the right track?

This is looking really good :) mmm tidier CSS

1. +++ b/core/modules/book/src/BookManager.php
   @@ -503,28 +503,22 @@ public function bookTreeOutput(array $tree) {
   -      $class = array();
   -      if ($i == 0) {
   -        $class[] = 'first';
   -      }
   -      if ($i == $num_items - 1) {
   -        $class[] = 'last';
   -      }
   

   Just making sure this is an intentional removal? I don't see how it fits within the scope of this issue.

2. +++ b/core/modules/book/src/BookManager.php
   @@ -503,28 +503,22 @@ public function bookTreeOutput(array $tree) {
   -        $class[] = 'leaf';
   +        $class[] = 'menu-item--leaf';
   

   Is the menu-item class supposed to be replacing the leaf class or extending it?

We are going to need some thorough visual regression testing

OH YES first and last removal was intentional.
RE:
1) It's not being used in CSS, already removed in menu build.
2) Extending. It's a type of menu-item (with no children)

Oh yeah should have mentioned. li.leaf's use was already covered by .menu li's CSS. Which is now covered by .menu-item... and that was it's only use in core. If there is an easy way to add it back through contrib (either extending MenuLinkTree::build() or doing it in the template...) and we can document that process then I think it would be safe to remove completely. It's an edge case indeed(when you need to style childless menu items different from ones with children, which you can do by targeting .menu-link--expanded/.menu-link--collapsed and excluding the styles on the children without it).

I've checked all of core for leaf, there was just that class and the class addition which is now .menu-item--leaf.

Fixed a couple of bugs in the toolbar and reviewed the changes a bit closer. Seems to be working out pretty well.

@mortendk hook me up with documentation on how we can achieve the .menu-item--leaf class without core and I'll gladly remove it in this patch... deal? Dare ya:P

Turkey! :P Re-uploading 13

@mortendk

I think people are able to do something like .menu-item .menu-item .menu-item to move down the structure of a menu, I don't think .leaf covers 80%

@LewisNyman yes and yes re #19 just doing some trading so we don't remove a "feature" without giving someone a fallback plan. .menu-item-leaf = .menu-item:not(.menu-item-collapsed):not(.menu-item-expanded)

OR the reverse just make the default .menu-item and override it on the other two which is the majority of cases.

Trying to get @mortendk to write the template example for determining and adding that class;) For good practice:P

We need a change record for this.

CR added here https://www.drupal.org/node/2427579

    /**
     * On page load, open the active menu item.
     *
     * Marks the trail of the active link in the menu back to the root of the
     * menu with .active-trail.
     *
     * @param {jQuery} $menu
     *   The root of the menu.
     */
    function openActiveItem($menu) {
      var pathItem = $menu.find('a[href="' + location.pathname + '"]');
      if (pathItem.length && !activeItem) {
        activeItem = location.pathname;
      }
      if (activeItem) {
        var $activeItem = $menu.find('a[href="' + activeItem + '"]').addClass('active');
        var $activeTrail = $activeItem.parentsUntil('.root', 'li').addClass('active-trail');
        toggleList($activeTrail, true);
      }
    }

This looks wrong now.

.expanded .locale-translation-update__wrapper {
  background: transparent url(../../../misc/menu-expanded.png) left .6em no-repeat;
}

This looks like this change might break it - depends if this is for an expanded details or menu... not sure.

I merged the change record for this issue with the one from #2425691: Remove .leaf/.menu-item--leaf from menu CSS components, per Alex's suggestion.

Morten, your patch file is empty.

Looks like changes to Seven's menus-and-lists from #17 when missing. I'm not seeing a comment that says it was intentional.

No idea why that file copy test is failing. I ran it locally and it passed after I made the Seven change. If that is the cause, good grief.

CSS is not frozen in beta. Committed 806def4 and pushed to 8.0.x. Thanks!

+++ b/core/themes/bartik/css/components/primary-menu.css
@@ -97,11 +97,11 @@ body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu-t
-body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu li {
+body:not(:target) .region-primary-menu .menu-toggle-target-show:target ~ .menu-item {

This selector change broke the toggle for the primary navigation in Bartik. I have opened a followup at #2430735: Primary navigation toggle broken in Bartik to fix this.