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:

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

mherchel’s picture

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

mherchel’s picture

Updating summary.

Gauravvvv’s picture

Assigned: Unassigned » Gauravvvv
Gauravvvv’s picture

/* 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’s picture

Status: Active » Needs review
Gauravvvv’s picture

Assigned: Gauravvvv » Unassigned
mherchel’s picture

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

Status: Needs work » Needs review
FileSize
1.37 KB

New patch attached that takes care of the issue.

mherchel’s picture

rikki_iki’s picture

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

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
1.31 MB
35.25 KB
23.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

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

Status: Needs work » Needs review
FileSize
2.86 KB
1.18 KB
chetanbharambe’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
423.68 KB
417.17 KB
429.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

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

FileSize
251.02 KB
245.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.

IndrajithKB’s picture

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)

mherchel’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
475.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

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

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

Status: Fixed » Closed (fixed)

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