Olivero primary menu JS completely breaks if site-builder specifies over 2 levels deep

Patch attached:

• removes 'primary-nav__menu-item--has-children' class from items that aren't menu_level 0
• removes data-drupal-selector="primary-nav-menu-item-has-children" from items that aren't menu_level 0
• prevents rendering dropdown arrow button from menu items that aren't menu_level 0

Screenshot:

New patch here going the javascript route.

This will leave all the menu-level-based classes and selectors in place for any future subtheming efforts. Most of the patch is setting up optional chaining on HTMLElement methods & properties (getAttribute, setAttribute, classList, etc). There's also some styling changes so deeper menu levels look a bit more consistent.

Please test extensively on desktop & mobile menus, and make note of any javascript errors originating from second-level-navigation.js or styles that look off, but on my machine this seems pretty clean.

Weird. new patch attached. Interdiff seems good though.

New patch with requested changes

Fixed custom command failed, Re-rolled patch #11.
Attached interdiff for same.

Your patch file is blank, mate.

@mherchel Thanks for the preview link! Just a couple of small design updates please:

1. Can we change the line-height of the 14px text in the subnav to 18px? I think 27px is too much...I'm reading it like another nav item, instead of a word wrap. When I change it in dev tools, it messes up some of the spacing though and I'd like to try to keep that intact.
2. The height of the blue underline is inconsistent. Can we keep this the same? Let's use the taller one.
3. Can we remove the vertical gray line after the 1st level? So it would appear for "Style Guide" but not appear for "Grid View" and "Comments".
4. Can we change the children items to Black 2? I have that as #313637 but may have changed with the color updates you did in the front-end. I think this will help create more hierarchy

Patch works and moving to RTBC.

There is a slight issue I'm seeing but I'm pretty sure it's not something we want to address in this issue: menu items with the same link text lead to duplicate IDs in the markup.

Marking as RTBC unless @mherchel thinks this is something we need to fix in this issue

Ah, I see the source. This is coming from the line:

{% set aria_id = (item.title ~ '-submenu-' ~ loop.index )|clean_id %}

This works fine when we don't support more than 1 submenu, but now we need to rethink this. We'll probably want to add a variable into the menu macro that we can append the value of loop.index onto. I'll work on this in the morning.

New patch builds the `aria_id` variable iteratively over menu levels ensuring items each have a unique id within the scope of the menu.

Adding related core issue about updates to .eslintrc

haven't done a indepth review but the use of ?. feel like code-smell to me, i might be wrong but there is an opportunity for some refactor to avoid having to use ?.

1. +++ b/core/themes/olivero/js/second-level-navigation.es6.js
   @@ -89,11 +89,11 @@
   +    button?.removeAttribute('aria-hidden');
   +    button?.removeAttribute('tabindex');
   

   This whole part seems like a delegated event listener would be a better solution instead of binding things individually like this.

2. +++ b/core/themes/olivero/js/second-level-navigation.es6.js
   @@ -89,11 +89,11 @@
   +    el?.addEventListener(
   

   here el is checked, but not above in the el.querySelector().

3. +++ b/core/themes/olivero/js/second-level-navigation.es6.js
   @@ -166,7 +166,7 @@
          const button = el.querySelector(
            '[data-drupal-selector="primary-nav-submenu-toggle-button"]',
          );
   -      const state = button.getAttribute('aria-expanded') === 'true';
   +      const state = button?.getAttribute('aria-expanded') === 'true';
    
          if (state) {
            subNavsAreOpen = true;
   

   this whole function might be refactorable with something like this maybe.

     function areAnySubNavsOpen() {
       return document.querySelector('[data-drupal-selector="primary-nav-menu-item-has-children"] [data-drupal-selector="primary-nav-submenu-toggle-button"][aria-expanded="true"]').length > 0;
     }
   

LGTM.

I tested on desktop and mobile, I created 3 levels menu.
There are no duplicate ids, and menu works well.

Thank you for creating the additional issues that will solve problems that are out of the scope of this issue.

I updated /admin/structure/block/manage/olivero_main_menu -> Menu Levels -> Number of levels to display
and set it to "Unlimited".

I was able to see all menu levels displayed. There's only 1 show/hide button that is added to the first level menu.

Would it be possible to add tests for this on top of the pre-existing test coverage?

#39 works perfectly, and we have the right pass/fail combo in the patches. Moving to RTBC!

Committed 364ecc6 and pushed to 9.3.x. Thanks!