[WNMGDS-2350] Fix background color of docs vertical nav on hover #2937
Conversation
| <Button | ||
| className="ds-u-md-display--none ds-u-padding-left--0 ds-u-padding-right--1" |
There was a problem hiding this comment.
removing ds-u-md-display--none from Button because the parent header is handling this logic
| @@ -106,9 +106,9 @@ const SideNav = ({ location }: SideNavProps) => { | |||
| })} | |||
| > | |||
| <UsaBanner className="ds-u-display--block ds-u-md-display--none" /> | |||
| <header className="c-navigation__header ds-u-md-display--block ds-u-md-display--none"> | |||
There was a problem hiding this comment.
removing the ds-u-md-display--block class as the following ds-u-md-display--none cancels it out.
| @@ -11,8 +11,13 @@ | |||
| gap: $spacer-2; | |||
| padding: $spacer-2; | |||
|
|
|||
| button { | |||
| > button { | |||
There was a problem hiding this comment.
made the button selector more precise here with the >
|
|
||
| &:hover, | ||
| &:focus:hover { | ||
| color: currentColor; |
There was a problem hiding this comment.
noticed the 🍔 icon is illegible when :focus:hover is applied and fixed that
There was a problem hiding this comment.
Oh yeah, I never noticed that because I must have always tested it in touch-device mode
| .ds-c-vertical-nav__label { | ||
| &:hover, | ||
| :visited:hover { | ||
| --vertical-nav-item__background-color--hover: color-mix( |
There was a problem hiding this comment.
all the code in the :before styles seemed to be in an attempt to modify an existing color token to be something we don't have (adding an alpha to whatever the vert nav hover bg was).
css filters are expensive in terms of overall perf, so removing that was a boon to us. also using color-mix() to accomplish the transparency needs.
side-note: i don't really think we should be combining/modifying tokens for one-off implementation, and the best solution here was probably to update this to just rely on --color-primary-lightest. i tried that out, but the color was noticeably different than our current color and decided to be less aggressive with the change. maybe add this to the pile for things to review if we update the docs site?
There was a problem hiding this comment.
Great analysis and solution! Very elegant
Sounds good to pile it onto future doc site changes 👍
| @@ -133,42 +138,23 @@ | |||
| } | |||
| } | |||
|
|
|||
| .ds-c-vertical-nav__label { | |||
There was a problem hiding this comment.
moved this rule outside of the desktop media query as the hover state should be available across viewport sizes
| // Desktop | ||
| @media (min-width: $media-width-md) { | ||
| .c-navigation { | ||
| // Set it to a fixed width that is wide enough for most links so that it | ||
| // doesn't jump around when we open and close subsections. | ||
| width: 17rem; | ||
|
|
||
| .c-navigation__header { |
There was a problem hiding this comment.
because the header doesn't appear on md+ sized viewports, this isn't needed
zarahzachz
added
Type: Fixed
| .ds-c-vertical-nav__label { | ||
| &:hover, | ||
| :visited:hover { | ||
| --vertical-nav-item__background-color--hover: color-mix( |
There was a problem hiding this comment.
Great analysis and solution! Very elegant
Sounds good to pile it onto future doc site changes 👍
WNMGDS-2350
I thought we needed to refactor our vertical nav CSS due to how complex the docs site CSS overrides were, but I was wrong and the solution turned out to be much simpler 🎉
:hover:focusstate of hamburger icon