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 a hover state to sidebar panel headers #10070

Merged
merged 4 commits into from
Sep 25, 2018

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Sep 20, 2018

This PR adds a hover state to sidebar panels. When a panel is closed:

  • The background changes to a light gray on hover
  • The text and icon change from $dark-gray-500 to $dark-gray-900

When a panel is already open, the text and icon color changes, but the background does not.

This helps subtly differentiate collapsed panels from uncollapsed ones, and provides another visual cue reinforcement that these headers are interactive.

This does lighten the un-hovered color of these headers to our usual $dark-gray-500 text color, but I don't think it effects their effectiveness as headers.

Screenshots

Closed, default:
screen shot 2018-09-20 at 9 46 17 am

Closed, hovered:
screen shot 2018-09-20 at 9 46 31 am

Open, default:
screen shot 2018-09-20 at 9 46 39 am

Open, title is hovered:
screen shot 2018-09-20 at 9 46 50 am

GIF

(The subtle background color isn't quite visible due to the compression 😕)

sidebar

This also works nicely alongside #10062:

sidebar-white

This PR adds a hover state to sidebar panels. When a panel is closed:

- The background changes to a light gray on hover
- The text and icon change from $dark-gray-500 to $dark-gray-900

When a panel is already open, the text and icon color changes, but the background does not.
@kjellr kjellr added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. labels Sep 20, 2018
@kjellr kjellr requested a review from jasmussen September 20, 2018 13:54
@kjellr kjellr mentioned this pull request Sep 20, 2018
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Love it.

We should make a few changes, then ship this. First off, the Revisions box should have the same hover:

screen shot 2018-09-21 at 09 28 44

Compare:

screen shot 2018-09-21 at 09 28 47

I don't know if we shouldn't change it here, but there's a change incoming in update/font-picker (not yet a PR) that changes the text color of the sidebar to be way darker, not just dark gray. This is to be consistent with headings & chevrons inside the block library.

I can be convinced otherwise, but it looks good in that branch.

@kjellr
Copy link
Contributor Author

kjellr commented Sep 21, 2018

We should make a few changes, then ship this. First off, the Revisions box should have the same hover

b023a78

I don't know if we shouldn't change it here, but there's a change incoming in update/font-picker (not yet a PR) that changes the text color of the sidebar to be way darker, not just dark gray. This is to be consistent with headings & chevrons inside the block library.

So, interestingly enough... it looks like as of right now, the Document Sidebar panel headers are dark-gray-900, but Block Sidebar panel headers (as well as the revisions header for some reason) are dark-gray-500:

artboard

That clearly doesn't make sense. 😄 As you mention, it looks like #9802 changes the block panel header color to dark-gray-900 to fix that.

I added that update to this branch in 13f5657. This technically works, but it's a little less ideal because now there's no hover state for expanded items. We could make the text lighter on hover, but that's the opposite of what happens with most of our other hover states.

In any case, give it a try and let me know what you think.

(Also — I realized these hover states go beyond the sidebar and effect the block picker, too! 😄 So be sure to take a look there too)

@jasmussen
Copy link
Contributor

I think this is pretty good!

I like the darker panel color. Also the hover seems fine in the library since we use it in a few places:

screen shot 2018-09-24 at 08 38 28

screen shot 2018-09-24 at 08 38 37

I don't think an expanded panel HAS to have that same hover, but it seems like just a matter of CSS specificity to make this work no? What makes this not work do you know?

@kjellr
Copy link
Contributor Author

kjellr commented Sep 24, 2018

I don't think an expanded panel HAS to have that same hover, but it seems like just a matter of CSS specificity to make this work no? What makes this not work do you know?

Oh right! I tried that initially, but decided that the margins around the title bar in some expanded panels felt too uncomfortable for my taste:

screen shot 2018-09-24 at 8 38 01 am

screen shot 2018-09-24 at 8 38 14 am

In those cases, it looks nice and comfortable without the gray background, but when the background is in place, the vertical margin in between the panel background and the panel content feels a little tight. That's why I explored a text-only hover state for expanded panels instead.

In any case, give this another spin and let me know if you think those margins are uncomfortable or not in practice. I could be convinced otherwise. 🙂

@jasmussen jasmussen self-requested a review September 25, 2018 07:15
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Ship it!

@kjellr kjellr merged commit af3153a into master Sep 25, 2018
@kjellr kjellr deleted the try/sidebar-panel-hover-states branch September 25, 2018 12:52
@kjellr kjellr added this to the 4.0 milestone Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants