Problem/Motivation

The menu links have a 36px bottom padding. This creates a large gap below the link item. As such, this feel like it violates the design principle of proximity (https://www.nngroup.com/articles/gestalt-proximity/). Indeed, the bottom border (on hover) seems a bit too detached from the menu item text that is corresponds to.

Steps to reproduce

  1. Install and Enable the Olivero Theme
  2. Hover over a top right menu link
  3. See the border is much lower than the link text

Here is a screenshot of what I am referring to: https://i.imgur.com/eSUbefG.png

Proposed resolution

I would propose shrinking the bottom padding significantly, to 16px.

Here is a screen shot of my proposed solution, implemented via dev tools: https://i.imgur.com/C9JWaiK.png

CommentFileSizeAuthor
#25 After Patch 3196408.png394.78 KBchetanbharambe
#25 Before Patch 3196408.png512.2 KBchetanbharambe
#13 3196408_after.png280.46 KBhinal05
#13 3196408_before.png281.68 KBhinal05
#12 after_patch.png259.06 KBvikashsoni
#12 Before_patch.png188.14 KBvikashsoni
#10 Screenshot 2021-03-03 at 10.04.52 AM.png12.84 KBlokeshsahu
#9 3196408-9.patch1.21 KBgauravvvv
#7 After--patch.jpg89.6 KBranjith_kumar_k_u
#6 3195828-6.patch1.48 KBgauravvvv
#2 Screenshot 2021-02-06 at 10.51.46.png9.39 KBgauravvvv
#2 3196408-2.patch613 bytesgauravvvv

Comments

lhockley created an issue. See original summary.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3196408-2.patch613 bytes
new Screenshot 2021-02-06 at 10.51.46.png9.39 KB

I have provided the patch please verify if it works or not. Padding bottom was fixed to 2.25 rem, which is too large, Now adding padding-bottom: 10px; as you suggested for 16px, But 10px seems good to me. Adding screenshot for reference. Please review.

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
lhockley’s picture

lhockley
Location Los Angeles
commented

I would certainly appreciate some feedback from others who have been more involved with the project.
That screenshot definitely looks acceptable to me, though. Thanks.

gauravvvv’s picture

gauravvvv commented
Version: 10.0.x-dev » 9.2.x-dev
gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3195828-6.patch1.48 KB
ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new After--patch.jpg89.6 KB

I have tested the last patch but the issue is still there
After patch
after patch

gauravvvv’s picture

gauravvvv commented

Have you cleared the cache? @ranjith_kumar_k_u

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3196408-9.patch1.21 KB

Sorry, I uploaded the wrong patch last time, Here I have provided the patch, Please review.

lokeshsahu’s picture

lokeshsahu commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2021-03-03 at 10.04.52 AM.png12.84 KB

I have tested the #9 patch and its working fine for me, have added an after patch screenshot for ref.

lokeshsahu’s picture

lokeshsahu commented
Status: Reviewed & tested by the community » Needs review
vikashsoni’s picture

vikashsoni commented
StatusFileSize
new Before_patch.png188.14 KB
new after_patch.png259.06 KB

Apply the patch #9 and working fine remove the Excessive Padding sharing the screenshot

hinal05’s picture

hinal05 commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3196408_before.png281.68 KB
new 3196408_after.png280.46 KB

Applied patch #9 and it's working fine. Please review the screenshot.

lhockley’s picture

lhockley
Location Los Angeles
commented

That is so much better! Thanks so much.

mherchel’s picture

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

This needs to go through design before any development. In the current implementation, the bottom of the menu aligns with the bottom of the site branding. I'm not sure we want to mess with that.

lhockley’s picture

lhockley
Location Los Angeles
commented

There hasn't been much response here. How do we get the attention of design on this issue?
Is there some other channel that this needs to be reported?

mherchel’s picture

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

There hasn't been much response here. How do we get the attention of design on this issue?

Yeah, honestly we've been concentrating on the stable blockers. There's no other channels, it's just a matter of priorities at this point. Ideally, this should have been brought up in #3088378: Designs for new front-end theme for Drupal 9. At this point in the process, we're more at the tail end of the implementation process.

lhockley’s picture

lhockley
Location Los Angeles
commented

I guess I was just a little bit late to the party. Priorities are understandable, though.

I guess there is nothing to prevent someone from making a child theme, although that would be quite overkill for such a small fix.

mherchel’s picture

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

Maybe. If you're considering subtheming, take a look at this guide I put up a bit ago at https://github.com/mherchel/olivero-subtheme

kiran.kadam911’s picture

kiran.kadam911
he/him
Location Mumbai, India 🇮🇳
commented
Issue tags: +duplicate

I think this issue will get cover in https://www.drupal.org/project/drupal/issues/3202667

So we can mark it as duplicate and close.

Thanks!

kiran.kadam911’s picture

kiran.kadam911
he/him
Location Mumbai, India 🇮🇳
commented
Status: Needs work » Needs review

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.

chetanbharambe’s picture

chetanbharambe commented
Assigned: Unassigned » chetanbharambe
chetanbharambe’s picture

chetanbharambe commented

Verified and tested patch #31.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: admin/structure/menu/manage/main?destination=/admin/structure/menu
# Create the Menus
# Save it
# Check the results

Expected Results:
# User should not see Excessive Padding

Actual Results:
# User is able to see the border is much lower than the link text

Note: This issue already covered in https://www.drupal.org/project/drupal/issues/3202667

Looks good to me.
Can be a move to RTBC.

chetanbharambe’s picture

chetanbharambe commented
Assigned: chetanbharambe » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before Patch 3196408.png512.2 KB
new After Patch 3196408.png394.78 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

Would be good to have confirmation on the design change from the Olivero maintainers.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Closed (won't fix)

The padding of the primary menu is part of the design. We're not changing this at this point.