Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
Comment | File | Size | Author |
---|---|---|---|
#24 | hover-states.gif | 475.46 KB | mherchel |
#19 | after--patch--pic.png | 245.13 KB | vikashsoni |
#19 | before-patch--pic.png | 251.02 KB | vikashsoni |
#17 | After Patch 3228801 Two.png | 429.97 KB | chetanbharambe |
#17 | After Patch 3228801 One.png | 417.17 KB | chetanbharambe |
Issue fork drupal-3228801
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:
Comments
Comment #3
mherchelIgnore previous comments and commits. I was demonstrating the new GitLab workflow.
Comment #4
mherchelUpdating summary.
Comment #5
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #6
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedWrong class target for removing border if submenu exists in #3226704
Comment #7
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #8
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #9
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #10
mherchelThanks 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.Try to figure out why the code is being added before just removing it.
This is invalid.
Comment #11
mherchelNew patch attached that takes care of the issue.
Comment #12
mherchelTugboat preview for the above patch: https://3228801-menu-hover-state-e5vjbnbpfr2s3p2wd93avqt7zxiarjd3.tugboa...
Comment #13
rikki_iki CreditAttribution: rikki_iki at PreviousNext commentedSelector looks correct in #11
Marking RTBC
Comment #14
ckrinaThanks 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 parenta.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 browsercursor
moves topointer
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 parenta
. So moving this back to Needs work.Comment #15
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedIn 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.
Comment #16
kostyashupenkoComment #17
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedVerified 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.
Comment #18
mherchelThanks for working on this! It works perfectly, but I have a couple code nits:
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.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
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.
Comment #19
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedApplied patch #16 working fine
after patch hower state is applied for ref sharing screenshot
Thanks for the patch
Comment #22
IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commentedHi @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 theoutline:0
and updated the code block with@nest
rule)Comment #24
mherchelThis 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:
Comment #28
lauriiiCommitted de073bc and pushed to 9.3.x. Cherry-picked to 9.2.x because Olivero is experimental. Thanks!
Comment #29
lauriii