Closed (fixed)
Project:
Navigation
Version:
1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
20 Nov 2023 at 16:53 UTC
Updated:
3 Jan 2024 at 12:24 UTC
Jump to comment: Most recent, Most recent file

Comments
Comment #2
pragati_kanade commentedLooking into this.
Comment #5
kostyashupenkoComment #6
finnsky commentedMaybe put this logic on sidebar level?
I personally not really like this:
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.
Comment #7
ckrinaI 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).
Comment #8
kostyashupenkoI 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
Comment #9
finnsky commentedRe #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.
Comment #11
finnsky commentedAdded imo simpler version. Please review!
Comment #12
kostyashupenko#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.
Comment #13
prashant.cIt 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.
Comment #14
finnsky commentedSo!
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!
Comment #15
ckrinaThe code looks good to me, and I've manually tested and works great. Thanks all!
Comment #17
prashant.cLooks 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.
Comment #20
kostyashupenkoThe 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
Comment #21
prashant.cI examined it in the live preview, and it functioned correctly.
Comment #23
ckrinaMerged! Thanks all for the work on this!