Problem/Motivation

Follow-up from
Olivero has 'main' and 'secondary' menus, the obvious choice is to put the 'navigation' menu in main and 'user account' menu in secondary.

When you have between 4-6 items in the main menu (not that many), a couple of items in the secondary menu (also not that many), and a 13" laptop monitor, it doesn't look great.

Opening as major since this is quite visually jarring right at the top of the page, and seems like an easy situation to get into, but could equally be 'normal'.

The previous approach to long menus in #3186992: Nav menu items can overflow outside of container , didn't look at the situation when there's something in the secondary menu, and that makes the situation worse - especially since word wrapping often means there's a lot of whitespace between the wrapped main menu and the secondary.

Steps to reproduce

Enable olivero and add several items to the navigation menu.

Proposed resolution

Not really my area, but a couple of ideas:

1. Change CSS/markup so that the secondary menu as a whole wraps under the main menu (but maybe still floated to the right) if things are too wide. If the navigation menu is still too wide even then, it would have to wrap to two lines too.

2. Javascript black magic to switch to the burger menu, even on desktop, if the main or secondary menu wraps.

From original report: Here is an example, reducing the window in width. When one of the menu links touches the logo area + no, it automatically switches to mobile mode :

https://www.clubic.com/

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#32 3246755-32-10.0.x-test-only-FAIL.patch1.79 KBmherchel
#32 3246755-32-10.0.x.patch7.72 KBmherchel
#31 3246755-31-10.0.x.patch7.7 KBmherchel
#30 after-less.gif2.47 MBcindytwilliams
#30 after.gif2.71 MBcindytwilliams
#30 before.gif3.08 MBcindytwilliams
#29 lessMenus.png51.44 KBlibbna
#29 desktop.png72.12 KBlibbna
#26 interdiff-25-26.txt2.15 KBmherchel
#26 3246755-26.patch7.82 KBmherchel
#25 interdiff-21.25.txt6.73 KBmherchel
#25 3246755-25.patch7.86 KBmherchel
#24 image (5).png35.6 KBlibbna
#24 image (4).png38.39 KBlibbna
#21 3246755-21-test-only-FAIL.patch1.78 KBmherchel
#21 3246755-21.patch6.6 KBmherchel
#16 3246755-16-test-only-FAIL.patch1.45 KBmherchel
#16 3246755-16.patch6.17 KBmherchel
#14 3246755-14-test-only-FAIL.patch1.45 KBmherchel
#14 3246755-14.patch6.24 KBmherchel
#11 image (3).png52.51 KBlibbna
#11 image (2).png81.72 KBlibbna
#10 laptop.png19.4 KBlibbna
#10 desktop.png38.47 KBlibbna
#9 interdiff-3246755-8-9.txt4.13 KByogeshmpawar
#9 3246755-9.patch3.99 KByogeshmpawar
#8 3246755.patch5.72 KBmherchel
Capture d’écran 2021-10-29 230345.png56.71 KBzenimagine
Capture d’écran 2021-10-29 230316.png59.1 KBzenimagine

Comments

zenimagine created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Version: 9.2.x-dev » 9.4.x-dev

I've been thinking about things like this for a bit. We could use something like resize observer to dynamically do this. However, doing so might be confusing for editors as they might not know why the site has mobile navigation.

zenimagine’s picture

zenimagine commented

This mode should be activated manually from the theme's preferences with some explanation. I find it less confusing than navigation on 2 lines

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

There's already an option to enable the mobile menu at all widths. How would this be different?

zenimagine’s picture

zenimagine commented

The option activates the mobile menu on all widths. I just want to activate it if the menu overflows with an option besides the one that already exists.

I want to keep my menu on widescreen, unless there is an overflow.

It would take a parameter for "If the main menu overflows":
"Display on 2 lines" OR "Switch to the mobile menu".

catch’s picture

catch
he/him
Primary language English
commented
Title: Provide an option to put the menu in mobile mode when it touches the logo + site name area » Olivero main/user account menu layout struggles with long menus
Category: Feature request » Bug report
Issue summary: View changes
Priority: Normal » Major

I just opened a duplicate of this at #3275440: Olivero main/user account menu layout struggles with long menus, recategorising as a bug since IMO this is a bit more of an obvious problem once you add a secondary menu and updating the issue summary with more details.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3246755.patch5.72 KB

Here's a patch that uses resize observer to watch for the change in format, and once detected it switches to the mobile menu.

It works pretty well, but won't support IE11 (which is probably okay).

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
StatusFileSize
new 3246755-9.patch3.99 KB
new interdiff-3246755-8-9.txt4.13 KB

Fixed CS issues.

libbna’s picture

libbna commented
StatusFileSize
new desktop.png38.47 KB
new laptop.png19.4 KB

Reviewed #9 patch and this is how it is looking. Added desktop and laptop view images.

Is this how it should look like? I don't see any changes, please correct me if I am wrong.

libbna’s picture

libbna commented
StatusFileSize
new image (2).png81.72 KB
new image (3).png52.51 KB

I also reviewed #8 patch and it is giving an expected result.
After adding 7 menu items, hamburger icon is visible. and if there are 6 or less menu items then all the menus can be seen in one line.
Added screen shots.

libbna’s picture

libbna commented
Status: Needs review » Reviewed & tested by the community
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work

Thanks for the feedback! Setting back to Needs Work because we still need tests. If anyone else wants to do additional manual testing (especially on other browsers), please do so.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3246755-14.patch6.24 KB
new 3246755-14-test-only-FAIL.patch1.45 KB
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

Looks like the test isn't passing.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3246755-16.patch6.17 KB
new 3246755-16-test-only-FAIL.patch1.45 KB

Ready for review!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work
libbna’s picture

libbna commented

Hi @mherchel This needs work or review?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

@Libbna Back to needs work. The tests are failing, plus I think there can be some additional optimizations.

libbna’s picture

libbna commented

okay.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3246755-21.patch6.6 KB
new 3246755-21-test-only-FAIL.patch1.78 KB

Updated patch with (hopefully) fixed tests

The last submitted patch, 21: 3246755-21.patch, failed testing. View results

The last submitted patch, 21: 3246755-21.patch, failed testing. View results

libbna’s picture

libbna commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new image (4).png38.39 KB
new image (5).png35.6 KB

Reviewed patch #21 and it is successfully working.
Added screenshots - before patch and after patch.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3246755-25.patch7.86 KB
new interdiff-21.25.txt6.73 KB

I did a bit of refactoring of this:

  • Fixed an edge case where if there are enough items to warrant always being in mobile nav - but the page was loaded in a small viewport and then widened, it *could* show the desktop navigation (with items wrapped)
  • Moved away from nested anonymous functions
  • More and better documentation
  • No need to check for the 'is-always-mobile-nav` theme setting

Setting this back to needs review because of the changes. Thank you!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3246755-26.patch7.82 KB
new interdiff-25-26.txt2.15 KB

Forgot to change a variable name. New patch (and interdiff) attached.

Status: Needs review » Needs work

The last submitted patch, 26: 3246755-26.patch, failed testing. View results

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review

Unrelated failure. Re-queuing tests.

libbna’s picture

libbna commented
StatusFileSize
new desktop.png72.12 KB
new lessMenus.png51.44 KB

I have reviewed in every aspect and #26 patch looks good to me.

Only I didn't get this point

Fixed an edge case where if there are enough items to warrant always being in mobile nav - but the page was loaded in a small viewport and then widened, it *could* show the desktop navigation (with items wrapped)

.

How can I test to prove this point?

cindytwilliams’s picture

cindytwilliams commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new before.gif3.08 MB
new after.gif2.71 MB
new after-less.gif2.47 MB

I also reviewed this patch, and it looks good to me, including this part:

Double check to see if the navigation is wrapping, and if so, re-enable mobile navigation. This solves an edge cases where if the amount of navigation items always causes the primary navigation to wrap, and the page is loaded at a narrower viewport and then widened, the mobile nav may not be enabled.

Before patch:

  • If there are a lot of menu items, they wrap on small desktops (doesn't look good) and display as hamburger on tablet/mobile.

After patch:

  • If there are enough menu items that would cause them to wrap on desktop, they will always be displayed as a hamburger menu.
  • Otherwise they display in a row on desktop, and as a hamburger menu on tablet/mobile.

Marking RTBC+1

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3246755-31-10.0.x.patch7.7 KB

Patch for 10.0.x attached. Same patch except no need to check for ResizeObserver.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3246755-32-10.0.x.patch7.72 KB
new 3246755-32-10.0.x-test-only-FAIL.patch1.79 KB

Apparently Nightwatch's resizeWindow() command straight up doesn't work in Drupal 10. It works fine in Drupal 9.4.x. Maybe different yarn dependencies? 🤷‍♂️🤷‍♂️🤷‍♂️

I'm attaching a new patch for Drupal 10 that replaces resizeWindow() with setWindowSize(), which does the same thing.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Looks like @nod_ ran into the same resizeWindow() issue in #3265617: Update Nightwatch to 2.x at https://www.drupal.org/project/drupal/issues/3265617#mr1958-note82049

Anywho, the 10.0.x patch above should be good to go (assuming tests pass 🤞🤞🤞)

The last submitted patch, 32: 3246755-32-10.0.x.patch, failed testing. View results

The last submitted patch, 32: 3246755-32-10.0.x.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

  • lauriii committed 0790449 on 10.0.x 
    Issue #3246755 by mherchel, yogeshmpawar, Libbna, cindytwilliams,...

  • lauriii committed 9fe1415 on 9.5.x 
    Issue #3246755 by mherchel, yogeshmpawar, Libbna, cindytwilliams,...

  • lauriii committed b2a4aaf on 9.4.x 
    Issue #3246755 by mherchel, yogeshmpawar, Libbna, cindytwilliams,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 0790449 and pushed to 10.0.x. Also committed #26 to 9.5.x and cherry-picked to 9.4.x. Thanks!

Status: Fixed » Closed (fixed)

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