Regression from #3226704: Olivero's top-level primary menu's hover states are not correct.

The dropdown submenus should have a hover state where the blue underlines grow in from the left.

This is how it should look like:

CommentFileSizeAuthor
#24 hover-states.gif475.46 KBmherchel
#19 after--patch--pic.png245.13 KBvikashsoni
#19 before-patch--pic.png251.02 KBvikashsoni
#17 After Patch 3228801 Two.png429.97 KBchetanbharambe
#17 After Patch 3228801 One.png417.17 KBchetanbharambe
#17 Before Patch 3228801.png423.68 KBchetanbharambe
#16 interdiff_11-15.txt1.18 KBkostyashupenko
#16 3228801-15.patch2.86 KBkostyashupenko
#14 hover-state.png23.82 KBckrina
#14 item.png35.25 KBckrina
#14 hover-menu-olivero.gif1.31 MBckrina
#11 3228801-11.patch1.37 KBmherchel
#7 3228801-7.patch2.13 KBgauravvvv
#4 Olivero_Theme_Demonstration_and_Zoom_Meeting.png126.86 KBmherchel

Issue fork drupal-3228801

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mherchel created an issue. See original summary.

mherchel opened merge request !1104

mherchel’s picture

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

Ignore previous comments and commits. I was demonstrating the new GitLab workflow.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes
StatusFileSize
new Olivero_Theme_Demonstration_and_Zoom_Meeting.png126.86 KB

Updating summary.

gauravvvv’s picture

gauravvvv commented
Assigned: Unassigned » gauravvvv
gauravvvv’s picture

gauravvvv commented
Related issues: +#3226704: Olivero's top-level primary menu's hover states are not correct 
/* Remove hover state if submenu exists. */
        & .primary-nav__menu-link-inner:after {
          content: none;
        }

Wrong class target for removing border if submenu exists in #3226704

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3228801-7.patch2.13 KB
gauravvvv’s picture

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

gauravvvv commented
Assigned: gauravvvv » Unassigned
mherchel’s picture

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

Thanks for the patch. This patch causes a regression on #3226704-5: Olivero's top-level primary menu's hover states are not correct. Plus the value for transform-origin is invalid.

  1. +++ b/core/themes/olivero/css/components/navigation/nav-primary-wide.pcss.css
@@ -17,9 +17,9 @@ body:not(.is-always-mobile-nav) {
+        /* & .primary-nav__menu-link-inner:after {

    Try to figure out why the code is being added before just removing it. 

  2. +++ b/core/themes/olivero/css/components/navigation/nav-primary-wide.pcss.css
@@ -209,6 +209,11 @@ body:not(.is-always-mobile-nav) {
+  transform-origin: scaleX(1);

    This is invalid.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3228801-11.patch1.37 KB

New patch attached that takes care of the issue.

mherchel’s picture

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

Tugboat preview for the above patch: https://3228801-menu-hover-state-e5vjbnbpfr2s3p2wd93avqt7zxiarjd3.tugboa...

rikki_iki’s picture

rikki_iki commented
Status: Needs review » Reviewed & tested by the community

Selector looks correct in #11

  • Top level menu items without children have hover state that's thick and grows from center
  • Top level menu items with children have no hover state
  • Second level menu items (inside dropdown) have hover state that's thin and grows from the left

Marking RTBC

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new hover-menu-olivero.gif1.31 MB
new item.png35.25 KB
new hover-state.png23.82 KB

Thanks everybody for working on this & reviewing it.

Testing the patch I noticed a weird behavior that could be improved in this same issue: the hover reacts when the child span.primary-nav__menu-link-inner is hovered, instead of when the parent a.primary-nav__menu-link--level-2 is. It causes that you can actually click&navigate without seeing the hover at any time; or a more visual detail: the the browser cursor moves to pointer without triggering the hover styles, which is confusing. Here's an small video showcasing it:

Maybe this screenshot is helpful:

Basically, I would expect the hover to react when I'm hovering the clickable area. Some possible solutions would be to change the element where the hover is applied, or make sure the span fills all the parent a. So moving this back to Needs work.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2021-09-14 at 9.50.44 AM.png49.27 KB
new Screenshot 2021-09-14 at 9.52.29 AM.png62.76 KB

In the current hover state, when we hover over child span span.primary-nav__menu-link-inner then the underline is showing only below the text area (inline).

If we do it for it's parent a.primary-nav__menu-link--level-2 , then the underline will be for block element (outside the text too).

Is this behavior do we need to implement @ckrina.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
StatusFileSize
new 3228801-15.patch2.86 KB
new interdiff_11-15.txt1.18 KB
chetanbharambe’s picture

chetanbharambe commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before Patch 3228801.png423.68 KB
new After Patch 3228801 One.png417.17 KB
new After Patch 3228801 Two.png429.97 KB

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

Testing Steps:
# Goto: Appearance -> Apply Olivero theme
# Goto: admin/structure/menu/manage/main?destination=/admin/structure/menu
# Add 3-4 submenu items under main menu
# User is not able to see blue underlines once Hovering on added submenus.

Expected Results:
# User should see blue underlines once Hovering on added submenus.

Actual Results:
# User is not able to see blue underlines once Hovering on added submenus.

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

mherchel’s picture

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

Thanks for working on this! It works perfectly, but I have a couple code nits: 

+++ b/core/themes/olivero/css/components/navigation/nav-primary.css
@@ -196,14 +196,13 @@
+  outline: 0

It looks like this outline: 0 does nothing and can be removed. I know it was there before, but honestly it looks like it's left over from something else. 

+++ b/core/themes/olivero/css/components/navigation/nav-primary.css
@@ -196,14 +196,13 @@
+  }

This code block can be re-written using the @nest rule within the CSS nesting spec that PostCSS supports.

Basically, you can do something like this

.primary-nav__menu-link-inner {
   ...

  &:after {
    ...

    @nest .primary-nav__menu-link:hover & {
      transform: scaleX(1);
    }
  }
}

Note that I didn't even know about this until recently, and we haven't used it in Olivero (or Drupal yet), but this is the perfect opportunity.

vikashsoni’s picture

vikashsoni commented
StatusFileSize
new before-patch--pic.png251.02 KB
new after--patch--pic.png245.13 KB

Applied patch #16 working fine
after patch hower state is applied for ref sharing screenshot

Thanks for the patch

Indrajith KB made their first commit to this issue’s fork.

Indrajith KB opened merge request !1221

indrajithkb’s picture

indrajithkb commented

Hi @mherchel , Thanks for the @nest rule, first time am get to know about the rule and am happy to apply the rule with Olivero theme. please review the updated MR (I have removed the outline:0 and updated the code block with @nest rule)

bnjmnm closed merge request !1104

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new hover-states.gif475.46 KB

This looks almost perfect! There's an extra line break that I'm going to remove, but because that's only a small change, I'm going to RTBC this.

Animated gif showing proper behavior:

lauriii made their first commit to this issue’s fork.

  • lauriii committed de073bc on 9.3.x 
    Issue #3228801 by mherchel, Indrajith KB, kostyashupenko, Gauravmahlawat...

  • lauriii committed b448d59 on 9.2.x 
    Issue #3228801 by mherchel, Indrajith KB, kostyashupenko, Gauravmahlawat...
lauriii’s picture

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

Committed de073bc and pushed to 9.3.x. Cherry-picked to 9.2.x because Olivero is experimental. Thanks!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 9.3.x-dev » 9.2.x-dev

Status: Fixed » Closed (fixed)

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