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

Try a white sidebar #10062

Merged
merged 3 commits into from
Sep 30, 2018
Merged

Try a white sidebar #10062

merged 3 commits into from
Sep 30, 2018

Conversation

jasmussen
Copy link
Contributor

This PR extracts an idea from #9784, a white sidebar.

This looks a little jarring at first, but can grow on you. Because it's sort of a jarring change, I think it needs to be tested in a PR, rather than be discussed. That way people can actually feel it, rather than look at screenshots.

What do you think?

Before:

screen shot 2018-09-20 at 11 21 01

After:

screen shot 2018-09-20 at 11 24 18

Especially in fullscreen mode this works:

screen shot 2018-09-20 at 11 26 06

This PR extracts an idea from #9784, a white sidebar.

This looks a little jarring at first, but can grow on you. Because it's sort of a jarring change, I think it needs to be tested in a PR, rather than be discussed. That way people can actually _feel_ it, rather than look at screenshots.

What do you think?
@jasmussen jasmussen added Needs Design Feedback Needs general design feedback. Needs Testing Needs further testing to be confirmed. labels Sep 20, 2018
@jasmussen jasmussen self-assigned this Sep 20, 2018
@jasmussen jasmussen requested a review from a team September 20, 2018 09:28
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I took it for a local test drive, as recommended!

My first instinct is indeed that it's just too much white and too spartan. I've thought we should put less white in the sidebar, so everything being that way makes it really tough for my eyes to lock onto different pieces of the editor.

I do think it has a benefit and that's having not-full-height-sidebars look more consistent:

2018-09-20 10 39 16

I like that, but when everything is scrolled down and the grey header is gone it all looks a bit indistinguishable:

2018-09-20 10 40 25

I think this all-white sidebar could work if the bottom remained white but the header was:

  1. a darker background
  2. stickied

So the "Document" and "Block" selector were always at the top. It's an important context to have and being able to scroll it out of view isn't great UX anyhow. We could fix its position (maybe restore its dark colour too) and I think this would actually be cool, even from Mr. I Don't Like White Sidebars Me.

@kjellr
Copy link
Contributor

kjellr commented Sep 20, 2018

Thanks for breaking this out into its own PR — this is helpful to see in action. I'm a little mixed on it initially, but leaning towards liking it. Here are a few first impressions:

  • I'm really into changing the bottom of the toolbar to white. I can't think of a convincing conceptual reason that should be gray.
  • I do think we need to adjust the Document/Block tab bar a bit. When it was gray, it was clear that the bar was hierarchically superior to the items below it, but right now it blends right in. Maybe we could introduce a much lighter gray background? Or a thicker bottom border for the whole bar? Or a darker bottom border?
  • I do wish there was a clearer distinction between different sidebar panels — they do all blend in with each other. But that's not based on anything new in this PR. I opened Add a hover state to sidebar panel headers #10070 with a minor change to hopefully help that along — I think it works nicely with this PR, too.

@jasmussen
Copy link
Contributor Author

Pushed a change in bd564cd

sticky light gray

Not sure I love it yet, but it addressed feedback from both of you: white bottom area, lighter gray tabs, sticky position.

If we do decide we dig it, we'd want to test this properly on mobile, including physical iphones, sticky positioning is always ugly on mobilesafari. We'd also want to use the z index mixin.

@kjellr
Copy link
Contributor

kjellr commented Sep 21, 2018

Interesting! I think I like the sticky, but it's not working for me at any breakpoints above 600px (I tried in Safari, Chrome, and Firefox). That seems to be different from your GIF above... I can see that the sticky property exists on larger screens, but it's not working. 🤔

In general though, I do really like the light gray bar, as well as the removal of the gray at the very bottom of the page. This seems like a reasonable update to me.

@jasmussen
Copy link
Contributor Author

Okay I addressed the feedback. Keeping design feedback label for now.

@karmatosed
Copy link
Member

I've been going back and forth on this. First try, have to admit didn't really like it. However, I have sat with it a bit and let the experience settle. I now love the latest iteration with the grey at the top. Great work everyone it really opens up the design and feels fresher.

@karmatosed karmatosed self-requested a review September 24, 2018 16:04
@jasmussen jasmussen added [Type] Enhancement A suggestion for improvement. and removed Needs Design Feedback Needs general design feedback. Needs Testing Needs further testing to be confirmed. labels Sep 25, 2018
@jasmussen jasmussen added this to the 4.0 milestone Sep 25, 2018
@jasmussen
Copy link
Contributor Author

🎉

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Tried this out locally and I dig it! Sorry for my delayed re-review. :shipit:

@jasmussen
Copy link
Contributor Author

Yay! Let's ship it!

@jasmussen jasmussen merged commit a5cad2b into master Sep 30, 2018
@jasmussen jasmussen deleted the try/white-sidebar branch September 30, 2018 10:27
@afercia
Copy link
Contributor

afercia commented Oct 3, 2018

Worth considering the sidebar header sticky position hides elements that are currently focused. This is a known issue with all similar fixed toolbars, especially when tabbing backwards. See in the screenshot below, where focus is on the publish date button, which is completely hidden behind the sidebar header:

screen shot 2018-10-03 at 17 46 10

@tofumatt
Copy link
Member

tofumatt commented Oct 3, 2018

Hmm, interesting point. Is the recommendation not to use a sticky menu then? It adds a lot of context to the menu. If it's a big problem though, please file an issue and we can consider removing the fixed position here, as long as it doesn't impact usability re: contrast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants