Problem/Motivation

There has been a regression: when a menu or submenu is opening, it should close the others. The reason is because the amount of vertical space is limited and menus can be really long, forcing to big scrolls.

Proposed resolution

When a menu/submenu is opened, the others should close so you never en up with more that one of them opened.

Issue fork navigation-3402899

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ckrina created an issue. See original summary.

pragati_kanade’s picture

Looking into this.

kostyashupenko made their first commit to this issue’s fork.

kostyashupenko’s picture

Status: Active » Needs review
finnsky’s picture

Maybe put this logic on sidebar level?

I personally not really like this:

            if (prevOpened && prevOpened !== button) {
              Drupal.behaviors.navigationProcessToolbarMenuTriggers.toggleButtonState(prevOpened, false);
            }

We have this logic for popovers with CustomEvents:
https://git.drupalcode.org/project/navigation/-/blob/1.x/js/sidebar.js?r...

I know that we search buttons twice. But this is payment for simple structure.

ckrina’s picture

Status: Needs review » Needs work

I just tested it and found something that should also be addressed: when you leave a submenu by opening another one (either clicking when opened, or hovering parent when the sidebar is collapsed) it should be reset. Meaning that every time that you open a submenu it should be in its initial stated (unless it’s active).

kostyashupenko’s picture

I would add my remark regarding javascript in module - we have many javascript files with too splitted logic from my point of view. For example expanding/collapsing behavior - we have different classnames and data attributes for 1st level buttons and 2nd level buttons. But actually the main goal of these buttons is just to expand/collapse, so these elements should be unified in this term and expanding behavior should be written in one drupal behavior (which controls all these buttons no matter on which levels they are).

I let @finnsky or other guys to complete this issue

finnsky’s picture

Re #8
In fact second level button is just expand/collapse first level one is much complicated in terms of hover popover and tooltips. that why i splitted this js.

finnsky’s picture

Status: Needs work » Needs review

Added imo simpler version. Please review!

kostyashupenko’s picture

Status: Needs review » Needs work

#7 not fixed.

When you expanding new menu, all child els should be collapsed by default unless it's an active element (but not sure if we have to check if element is active or not). I'm thinking that by default element is already active and all its tree is expanded. Means if user decided to collapse parent menu -> child should be collapsed aswell no matter if it's active element or not.

prashant.c’s picture

StatusFileSize
new5.51 MB

It is concurred with #12 that in instances where the parent menu undergoes a collapse, the associated child menu items should likewise undergo a collapse, specifically if they are not currently in an active state.

finnsky’s picture

Status: Needs work » Needs review

So!

In my point of view Component (Popover, Menu, Tooltip) only should control own state.
And all common logic will stay on sidebar.js level.
All communication between Navigation Components happens in one place via Custom Events bubbles.

Please review!

ckrina’s picture

Status: Needs review » Reviewed & tested by the community

The code looks good to me, and I've manually tested and works great. Thanks all!

prashant.c’s picture

Looks good to me. Working as expected for Parent menus. I am assuming we will not collapse child menus when other one is expanded for example under "Configuration" menu.

kostyashupenko changed the visibility of the branch close-all-when-one-opened-3402899 to hidden.

kostyashupenko changed the visibility of the branch 3402899-one-menu-opening to hidden.

kostyashupenko’s picture

Status: Reviewed & tested by the community » Needs review

The fastest option was to create new MR instead of doing hard rebases.
MR 151 opened, and i moved manually all the changes from the previous MR 141. Previous MR worked perfectly and i wanted to merge it and change status to "Fixed", but since hard rebase / recreation of the new MR -> this issue needs review again

prashant.c’s picture

I examined it in the live preview, and it functioned correctly.

ckrina’s picture

Status: Needs review » Fixed

Merged! Thanks all for the work on this!

Status: Fixed » Closed (fixed)

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