Problem/Motivation

Once the combined width of menu items exceed the available container width, they overflow outside their intended home.

When this happens, the page becomes horizontally scrollable, and attempts to horizontally scroll result in a redraw loop, which can be seen in this video: overflow-glitch.mov

Bartik addresses this by wrapping menu items to a new line if needed. It's quite possible there's a more elegant way to address this. If a more elegant solution exists I'm confident the folks working on Olivero will find it 🙂.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#41 Capture d’écran 2021-10-29 230345.png56.71 KBzenimagine
#41 Capture d’écran 2021-10-29 230316.png59.1 KBzenimagine
#31 After Patch 3186992.png341 KBchetanbharambe
#31 Before Patch 3186992 up.png357.23 KBchetanbharambe
#30 Patch NA 3186992.png521.33 KBchetanbharambe
#30 Before Patch 3186992.png391.95 KBchetanbharambe
#29 interdiff_19-29.txt872 byteskiran.kadam911
#29 3186992-29.patch2.03 KBkiran.kadam911
#23 3186992-after.png14.82 KBabhijith s
#23 3186992-before.png18.57 KBabhijith s
#22 After-patch.png503.73 KBsakthivel m
#22 Before-patch.png485.86 KBsakthivel m
#19 3186992-19.patch2.05 KBtushar_sachdeva
#15 After-patch.png88.21 KBbhumikavarshney
#15 Before-patch.png85.28 KBbhumikavarshney
#14 3186992_14.patch2.54 KBhinal05
#13 3186992_13.patch2.7 KBhinal05
#13 3186992_after_scrollnav_13.png293.57 KBhinal05
#13 3186992_after_13.png371.64 KBhinal05
#13 3186992_after_scrollnav_9.png286.75 KBhinal05
#13 3186992_after_9.png333.47 KBhinal05
#13 3186992_before.png303.13 KBhinal05
#10 olivero-main-menu.png83.09 KBbabusaheb.vikas
#9 3186992-9.patch863 bytesdjsagar
#6 3186992-6.patch426 bytesdjsagar
#6 Screen Shot 2020-12-16 at 5.09.38 PM.png191.48 KBdjsagar
#4 After--patch-RTL.jpg294.22 KBranjith_kumar_k_u
#4 After--patch-LTR.jpg279.65 KBranjith_kumar_k_u
#4 Screenshot-from-2020-12-15.jpg227.72 KBranjith_kumar_k_u
#2 menu-overflow.png187.33 KBhitvika_verma
#2 menu-overflow.3186992.2.patch19.45 KBhitvika_verma
overflow-glitch.mov3.02 MBbnjmnm
nav-menu-overflow.png87.85 KBbnjmnm

Comments

bnjmnm created an issue. See original summary.

hitvika_verma’s picture

hitvika_verma commented
Status: Active » Fixed
StatusFileSize
new menu-overflow.3186992.2.patch19.45 KB
new menu-overflow.png187.33 KB
anmolgoyal74’s picture

anmolgoyal74 commented
Status: Fixed » Needs review
ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new Screenshot-from-2020-12-15.jpg227.72 KB
new After--patch-LTR.jpg279.65 KB
new After--patch-RTL.jpg294.22 KB

I have tested the above patch on 9.2,the patch applied cleanly but it does not fix the issue.

patch applying

After patch LTR
after patch LTR

After patch RTL
after patch RTL

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
Status: Needs review » Needs work
djsagar’s picture

djsagar commented
StatusFileSize
new Screen Shot 2020-12-16 at 5.09.38 PM.png191.48 KB
new 3186992-6.patch426 bytes

Patch is resolved this issue please see the screen short for clarification.

Thanks!

djsagar’s picture

djsagar commented
Status: Needs work » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work

I noticed all the patches are making direct edits to .css files that begin with this warning

/*
 * DO NOT EDIT THIS FILE.
 * See the following change record for more information,
 * https://www.drupal.org/node/3084859
 * @preserve
 */

Edits should be made to the corresponding .pcss.css file, and compiled. Running yarn watch while you edit the .pcss.css files should be sufficient.

I'd also recommend checking with the Olivero designers as they may want this to be fixed in a specific way. The links overflowing outside of the header like they do in #6 probably isn't what they had in mind.

djsagar’s picture

djsagar commented
Status: Needs work » Needs review
StatusFileSize
new 3186992-9.patch863 bytes

Hi @bnjmnm,

Thank you for your feedback.

I re-uploaded patch by running yarn watch.

Please review.

babusaheb.vikas’s picture

babusaheb.vikas commented
StatusFileSize
new olivero-main-menu.png83.09 KB

I tested 3186992-9.patch patch with extra menu items. For alignment working good. But after scroll, the page down the menu items design is breaking. That is not looking good.
Please find the screenshot.

babusaheb.vikas’s picture

babusaheb.vikas commented
Status: Needs review » Needs work
hinal05’s picture

hinal05 commented
Assigned: Unassigned » hinal05
hinal05’s picture

hinal05 commented
Assigned: hinal05 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3186992_before.png303.13 KB
new 3186992_after_9.png333.47 KB
new 3186992_after_scrollnav_9.png286.75 KB
new 3186992_after_13.png371.64 KB
new 3186992_after_scrollnav_13.png293.57 KB
new 3186992_13.patch2.7 KB

Applied patch #9 and also updated new patch for navigation scroll alignment. Please review it.

@babusaheb.vikas : I have applied the patch for scrolling menu but it's not proper solution for default theme. I have did override padding for specific scrolled navigation but it's just for some limited links (like max 2 lines). After adding more links(more than 2 lines) it will breaking again. so, as per requirement need to override css.

Main menu show for clear and concise, need to change navigation as per below suggestions.

- Reduce number of top level links
- Reduce length of link titles
- Reconsider menu hierarchy
- Reduce font size
- Remove 'Home' link because if you clicking on the logo in your Header will take you back to the homepage.

hinal05’s picture

hinal05 commented
StatusFileSize
new 3186992_14.patch2.54 KB

Please review updated patch.

bhumikavarshney’s picture

bhumikavarshney commented
StatusFileSize
new Before-patch.png85.28 KB
new After-patch.png88.21 KB

Applied patch #14 and it works fine.Adding screenshots below.

imalabya’s picture

imalabya
Primary language English
Location Bangalore
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs subsystem maintainer review

Looks good to me, Moving to RTBC

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/olivero/css/components/navigation/nav-primary-wide.pcss.css
@@ -55,13 +55,17 @@ body:not(.is-always-mobile-nav) {
+    & .js-fixed .primary-nav__menu-link-inner {

We shouldn't use js- prefixed classes for styling since they are only supposed to be used for functionality. For more info, see #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality.

tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: Unassigned » tushar_sachdeva
tushar_sachdeva’s picture

tushar_sachdeva commented
StatusFileSize
new 3186992-19.patch2.05 KB

@lauriii removed 

@@ -55,13 +55,17 @@ body:not(.is-always-mobile-nav) {
+    & .js-fixed .primary-nav__menu-link-inner {

as it was not affecting menu because .js-fixed class comes into play when menu is closed. Quick and easy patch applied .

tushar_sachdeva’s picture

tushar_sachdeva commented
Status: Needs work » Needs review
tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: tushar_sachdeva » Unassigned
sakthivel m’s picture

sakthivel m commented
StatusFileSize
new Before-patch.png485.86 KB
new After-patch.png503.73 KB

Applied patch #19 and it works fine. Adding screenshots below.

1+ RTBC

abhijith s’s picture

abhijith s commented
StatusFileSize
new 3186992-before.png18.57 KB
new 3186992-after.png14.82 KB

Applied patch #19 and it worked fine.

Before patch:
before

After patch:
after

RTBC +1

kiran.kadam911’s picture

kiran.kadam911
he/him
Location Mumbai, India 🇮🇳
commented
Status: Needs review » Reviewed & tested by the community

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3186992-19.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Restoring status after HEAD was broken.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3186992-19.patch, failed testing. View results

kiran.kadam911’s picture

kiran.kadam911
he/him
Location Mumbai, India 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new 3186992-29.patch2.03 KB
new interdiff_19-29.txt872 bytes

Re-Rolled with 9.3.x

Kindly review the attached patch.

Thanks!

chetanbharambe’s picture

chetanbharambe commented
Status: Needs review » Needs work
StatusFileSize
new Before Patch 3186992.png391.95 KB
new Patch NA 3186992.png521.33 KB

Hi @kiran,
Verified and tested patches #29.
The patch does not apply successfully.

Please refer attached screenshots.
Moving to Needs work.

chetanbharambe’s picture

chetanbharambe commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new Before Patch 3186992 up.png357.23 KB
new After Patch 3186992.png341 KB

Hi, @kiran Thank you for the patch.
Verified and tested patch #29.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance: Apply Olivero theme
# Goto admin/structure/menu
# Add 7-8 menus under the main navigation
# See the results

Expected Results:
# Navigation menu items should not overflow outside of container.

Actual Results:
# Navigation menu items can overflow outside of container.

Please refer attached screenshots.
Looks good to me.
Can be a move to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 3186992-29.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Reviewed & tested by the community

Unrelated test failure pushed this out of RTBC. Switching back to the RTBC @chetanbharambe set in #31, with no additional review from me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 3186992-29.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Reviewed & tested by the community

Unrelated test failure set this to needs work. Switching back no RTBC based on #31 - no additional review from me

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@lauriii and I discussed this and agreed that this can be committed once the 9.3 alpha freeze is over.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs subsystem maintainer review

Reviewed this issue and I think this is ready to be committed after the alpha freeze ends.

Removing the needs subsystem maintainer review tag since I don't think this actually needs subsystem maintainer review.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 4063e76cbe to 9.4.x and beb3bf6e6c to 9.3.x. Thanks!

Backported to 9.3.x as olivero is not stable yet and might be marked stable in 9.3.0.

  • alexpott committed 4063e76 on 9.4.x 
    Issue #3186992 by hinal05, djsagar, kiran.kadam911, hitvika_verma,...

  • alexpott committed beb3bf6 on 9.3.x 
    Issue #3186992 by hinal05, djsagar, kiran.kadam911, hitvika_verma,...
zenimagine’s picture

zenimagine commented
StatusFileSize
new Capture d’écran 2021-10-29 230316.png59.1 KB
new Capture d’écran 2021-10-29 230345.png56.71 KB

@alexpt I think this functionality is incomplete. There should be an option in the Olivero theme configuration to set the menu to 2 lines or to put the menu in mobile mode when the links touch the logo + name area.

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/

In my case (see screenshots), I would like that when the menu touches the gray area (logo + site name), it automatically switches to mobile mode.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@zenimagine can you open a follow-up issue for that?

zenimagine’s picture

zenimagine commented
Related issues: +#3246755: Olivero main/user account menu layout struggles with long menus

yes

mherchel’s picture

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

Opened up #3247269: Olivero: Alignment of primary menu hover states and dropdowns is incorrect at wide widths as a regression to this issue.

Sorry I've been out for a bit, I've been meaning to review this but a good amount of personal stuff got in the way.

Status: Fixed » Closed (fixed)

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