Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add background color to menu items being hovered #13732

Merged
merged 4 commits into from
Feb 13, 2019
Merged

Conversation

jasmussen
Copy link
Contributor

This PR adds some clarity to which menu item you're highlighting. It:

  • Adds a background color to a menu item hovered
  • Increases the contrast (opacity) of the keyboard shortcut indicator when hovering
  • It widens the hit are of the button to go edge to edge

It's a relatively small PR, but it's a really nice improvement to have.

Before:

screenshot 2019-02-07 at 12 42 30

After:

screenshot 2019-02-07 at 12 56 56

GIF:

after

@jasmussen jasmussen added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Feb 7, 2019
@jasmussen jasmussen self-assigned this Feb 7, 2019
@youknowriad
Copy link
Contributor

If I'm not mistaken, it feels a little bit darker than the background used for the inserter/switcher pills. Should we align both?

Otherwise, LGTM

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@jasmussen
Copy link
Contributor Author

You're referring to the hover effect on blocks in the library and switcher right? Yes, eagle eye! That was $light-gray-100, this is $light-gray-200.

I have darkened others that use that light gray a little bit:

screenshot 2019-02-07 at 15 31 45

screenshot 2019-02-07 at 15 31 50

screenshot 2019-02-07 at 15 31 54

@youknowriad
Copy link
Contributor

Should we add the same hover styles to the block navigation menu?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@chrisvanpatten
Copy link
Contributor

What's the a11y contrast here? Are we within an acceptable ratio?

@jasmussen
Copy link
Contributor Author

What's the a11y contrast here? Are we within an acceptable ratio?

Well within!

screenshot 2019-02-08 at 08 56 38

@jasmussen
Copy link
Contributor Author

Should we add the same hover styles to the block navigation menu?

You mean the block toolbar?

@kjellr
Copy link
Contributor

kjellr commented Feb 11, 2019

In general, this looks good to me. But I'm noticing that these styles aren't being picked up in the slash inserter:

select

@youknowriad
Copy link
Contributor

You mean the block toolbar?

I mean the block hierarchy menu, where we list all the blocks.

Joen Asmussen and others added 2 commits February 12, 2019 11:10
This PR adds some clarity to which menu item you're highlighting. It:

- Adds a background color to a menu item hovered
- Increases the contrast (opacity) of the keyboard shortcut indicator when hovering
- It widens the hit are of the button to go edge to edge

It's a relatively small PR, but it's a really nice improvement to have.
@jasmussen
Copy link
Contributor Author

Great catches, both. Rebased, then fixed the issues:

hover

@kjellr
Copy link
Contributor

kjellr commented Feb 12, 2019

Noticed one more tiny thing: The font size dropdown doesn't include this hover state:

screen shot 2019-02-12 at 8 34 20 am

Other than that, this looks great though.

@jasmussen jasmussen merged commit a99c0b5 into master Feb 13, 2019
@jasmussen jasmussen deleted the try/menu-hover-color branch February 13, 2019 13:30
@youknowriad
Copy link
Contributor

You're too quick @jasmussen :P I just noticed that the background is not applied on small breakpoints in the more menu.

@jasmussen
Copy link
Contributor Author

Haha ack. I'll make a follow-up PR in a few minutes.

@jasmussen
Copy link
Contributor Author

Actually wait, I think that's intentional. I recall a comment in the JS file where it's commented out because of some performance issue. Let me find it.

@jasmussen
Copy link
Contributor Author

@youknowriad
Copy link
Contributor

Interesting 🤔

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Add background color to menu items being hovered

This PR adds some clarity to which menu item you're highlighting. It:

- Adds a background color to a menu item hovered
- Increases the contrast (opacity) of the keyboard shortcut indicator when hovering
- It widens the hit are of the button to go edge to edge

It's a relatively small PR, but it's a really nice improvement to have.

* Darken other hover colors to match.

* Address feedback.

* Apply to font size picker too.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Add background color to menu items being hovered

This PR adds some clarity to which menu item you're highlighting. It:

- Adds a background color to a menu item hovered
- Increases the contrast (opacity) of the keyboard shortcut indicator when hovering
- It widens the hit are of the button to go edge to edge

It's a relatively small PR, but it's a really nice improvement to have.

* Darken other hover colors to match.

* Address feedback.

* Apply to font size picker too.
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants