Search URL hover background color Inconsistent to other Menu Dropdowns

wpdarren commented 2 years ago

Bug Description

Minor UI issue. When entering a term into the search URL field, the results appear.

When you hover over the results, the background colour is darker than the other menu options.

See screencast below.

search

Steps to reproduce

Go to 'Site Kit dashboard'
Click on 'Search URL' icon in the header
Type in a search term
Results appear see issue when you hover over items.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

The background color for hovered menu items should be consistent across all menus in the header
- The background color of the search results menu should be updated to match the color used by the date range select

Implementation Brief

Using /assets/sass/config/_variables.scss :
- Introduce a new style color variable $c-list-item-bg-hover: rgba($c-black, 0.04)
- Replace the value of the variable $c-entity-search-autocomplete-bg-hover with this new variable

Test Coverage

No new tests are needed

QA Brief

Changelog entry

aaemnnosttv commented 1 year ago

@sebastianmantel would you please advise to the proper token we should use for the menu item's background color on hover?

sebastianmantel commented 1 year ago

@wpdarren @aaemnnosttv The hover state that is being used on the date picker is the right one.

sebastianmantel commented 1 year ago

@wpdarren @aaemnnosttv Hover state token should be N-50. I just reviewed the whole UI and the only place where is different is in the search component. Let me know if you find another issue and i'll take a look.

techanvil commented 1 year ago

Hi @sashadoes, thanks for the IB here. It's close, but not quite right.

Bearing in mind, the colour we're trying to match is that of the .mdc-list-item, which when hovered can be seen to have the background colour #000 with opacity 0.04.

This effectively results in a background colour with hex code #f5f5f5. The gray-0 colour you've identified has the hex code #f6f7f7, so it's very slightly off.

What I'd suggest here, is to create a new SCSS variable to represent the list item colour, called $c-list-item-bg-hover. This can have the value rgba($c-black, 0.04) to be defined in a way that corresponds to the the MDC style.

This can then be used in the _googlesitekit-entity-search.scss as described.

I would also suggest that we can leave _googlesitekit-autocomplete.scss alone, as this styling is overridden by that in _googlesitekit-entity-search.scss anyway. It doesn't look like the style _googlesitekit-autocomplete.scss is used anywhere and maybe it would be better to only define this background colour in _googlesitekit-autocomplete.scss as a globally applied style, but I think if we're to do this we should also remove the style from _googlesitekit-entity-search.scss at which point it starts to feel a bit out of scope for the issue. Rather I think, let's leave _googlesitekit-autocomplete.scss alone and we can tidy it up when we move onto the GM3 implementation.

Lastly, please can you add an estimate?

techanvil commented 1 year ago

Adding a followup note to say, I had not spotted @sebastianmantel's comment about using the N-50 token until now, however, this would not actually meet the AC. The N-50 token is defined as #ebeef0 which is not the same colour used for the dropdown menu hover, and is noticeably darker.

Menu:

Search result using N-50:

Therefore in order to match the dropdown the correct colour to use is indeed rgba($c-black, 0.04) as specced. We should use this for now, and we can address using a defined GM3 token for the hover state as part of the forthcoming GM3 branding work.

@sashadoes, one additional point, I realised we should simply replace the value of $c-entity-search-autocomplete-bg-hover in _variables.scss rather than change to a different variable altogether, so have updated the IB.

techanvil commented 1 year ago

IB :white_check_mark:

aaemnnosttv commented 1 year ago

Introduce a new style color variable $c-list-item-bg-hover: rgba($c-black, 0.04)

@techanvil I'm not sure we should introduce a new color token that's outside of our Tokens or at least part of our existing defined color palettes.

What about using Tertiary Hover here? Let's ask Sigal to advise if there isn't an obvious existing choice to go with.

techanvil commented 1 year ago

Hi @aaemnnosttv, while I do share your concern about using a colour outside of our defined tokens or colour palette, I think we should also be pragmatic here in the choice that we make.

It's worth bearing in mind that the proposed colour rgba(0, 0, 0, 0.04) is currently in use in Site Kit in a variety of places for the hover state including:

The date range dropdown:

The Dashboard Settings chips:

The various dropdowns in the Settings page:

I would therefore suggest that it's actually a de-facto part of our existing palette, it's just not something we've defined as a variable.

There is a subtle but clear difference when comparing Tertiary Hover to the list item hover:

I do think that overall design consistency should be the priority here rather than avoiding introducing an extra colour variable.

With all this in mind, I think we should take one of two routes here:

Either, we do align the search result hover to be aligned with the existing hover states by setting it to rgba(0, 0, 0, 0.04), and we can subsequently ensure they are all using a token value when we get around to the GM3 rebranding.
Alternatively, we do a more involved change whereby we update all of the current usage of this colour (as outlined above, and any additional usages) to instead use Tertiary Hover (this being the closest matching token colour).

What do you think, or should we ask Sigal for her input here?

aaemnnosttv commented 1 year ago

Thanks for the breakdown @techanvil. Let's ask @sigal-teller about this as I feel we should try to fit this into our design system if possible. It might be worth looking at the GM2 styles to see if this already refers to a token in the old system.

If this turns into a bigger issue which addresses a larger inconsistency throughout, I think that's okay. It's probably not worth a large effort but I don't see it becoming a difficult change to make, we just need to better understand what the correct application is for us.

sigal-teller commented 1 year ago

@aaemnnosttv @techanvil I see that we have some inconsistencies for the hover colors. As for the dropdowns, I checked the GM2+ design system and there isn't any specific definition for dropdowns hovers. However, I did find this I think we should use the "tertiary-hover" token, as defined in this example as hover, and use it everywhere - tertiary buttons hover, dropdowns hovers and any other component with white bg that changes to gray in hover state.

ivonac4 commented 8 months ago

@techanvil Seems like the next step is on you here. Are you still planning to pick it up anytime soon?

techanvil commented 4 months ago

@ivonac4 sorry for the slow reply here! As I've not managed to get to this and am pretty busy with other things, I've unassigned this so someone else can pick it up if they have capacity.

google / site-kit-wp