-
Notifications
You must be signed in to change notification settings - Fork 127
Fix BrandNavbar burger doesn't show submenus #2242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix BrandNavbar burger doesn't show submenus #2242
Conversation
Summary of ChangesHello @stefanocudini, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical display and interaction bug within the BrandNavbar component, specifically concerning its burger menu and submenus. The changes ensure that when the navigation bar is collapsed, submenus are properly styled and can be interactively opened and closed by users, significantly improving the user experience for mobile and smaller screen sizes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
fix click events brad_navbar fix paddinds in submenus fix style in less
951620d to
82373c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully fixes an issue where submenus in the collapsed 'burger' navigation bar were not functional. The approach involves adding a JavaScript click handler to toggle the visibility of submenus and adjusting the CSS to ensure they are displayed correctly within the collapsed menu structure. The changes are logical and effective. My review includes a few suggestions for cleaning up leftover code from development, such as commented-out blocks and a console.log statement, to improve code clarity and maintainability. I also noted the use of !important in the CSS, which could be revisited for better long-term maintainability.
| }); | ||
| } | ||
| } | ||
| // } else { // NOTE: This condition is never reached. ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @dsuren1
I saw your code added here.
I can't replicate the use of the condition in else.
I have a menu configured by admin to have custom menus with sub-items.
But this block never seems to be used.
I'd like to make sure I'm not missing a specific condition in the menu configuration.
b62019b to
1329c72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanocudini
Thanks for the PR. I believe the change mentioned below should be sufficient to achieve the desired outcome without modifying the expected behavior or styling
Lines 112 to 117 in 70bfa4c
| const clone = item.cloneNode(true); | |
| if (clone.tagName === 'LI') { | |
| menuBottomUl.appendChild(clone); | |
| } else { | |
| const items = clone.querySelectorAll(':scope > li'); | |
| items.forEach(item => { |
Change required
const clone = item.cloneNode(true);
+ const hasDropdown = clone.tagName === 'LI' && (clone.querySelector('.dropdown') || clone.querySelector('.dropdown-toggle'));
- if (clone.tagName === 'LI') {
+ if (clone.tagName === 'LI' && !hasDropdown) {
menuBottomUl.appendChild(clone);
} else {
- const items = clone.querySelectorAll(':scope > li');
+ const items = clone.tagName === 'LI' ? [clone] : clone.querySelectorAll(':scope > li');Expected output
hi @dsuren1 thankyou for the suggestions, the rest internal logic in
|
Yes, I believe that is the behavior it was intended to have if it were functioning correctly. The idea of adding a collapsible menu button inside the dropdown is good, but it should be reviewed. @giohappy and @allyoucanmap your thoughts on this? |
|
@dsuren1 @stefanocudini for the moment I would prefer to keep the current behavior without button to expand the menu. |
|
@allyoucanmap Thanks for the clarification. @stefanocudini Could you kindly update the PR to clean up commented out code. Thanks! |
|
@dsuren1 sorry again, I had an exchange with @stefanocudini and we will try his proposal, I just noticed the video in description |
* dropdown click fix click events brad_navbar fix paddinds in submenus fix style in less * remove importans * clean commented code (cherry picked from commit 3cc566b)
* dropdown click fix click events brad_navbar fix paddinds in submenus fix style in less * remove importans * clean commented code (cherry picked from commit 3cc566b) Co-authored-by: Stefano Cudini <[email protected]>

Fix follow problem, submenu items in custom menus


hidden only in collapsed menu button
Some fix inside .gn-brandmenu-bottom (collapsed menu)
Result of the PR:
submenu.mp4
For testing this fix is require to creare some menu and menu ites by admin interface, placed in TOPBAR_MENU_LEFT


NOTES: