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

Fix issue with unified toolbar not fitting #10990

Merged
merged 2 commits into from
Oct 25, 2018
Merged

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 24, 2018

Fixes #10959.

This PR fixes an issue where at certain viewport widths, there wouldn't be room for the unified toolbar between the icons on the left, and the buttons on the right, in the editor bar. It does so by:

  • Reducing the padding between toolbar segments
  • Introducing a new breakpoint, xlarge (1080px), that allows the toolbar to stay in the editor bar for longer, before it stacks.

Note, it also changes the design a smidgeon. CC @youknowriad for opinions, I know this reverts a decision you made earlier:

after option 2

The reason being: the edge to edge borders are there to imply regions of the interface. This change makes them more like separators between chunks of content, which I feel speaks better to what it is. CC: @alexislloyd and @karmatosed for thoughts.

But my opinion on this is not STRONG, and I can also trivially change it to this, if that's preferred — in fact it may be necessary due to the UI freeze:

after option 1

How it looked before:

GIF of the fix:

fix

Fixes #10959.

This PR fixes an issue where at certain viewport widths, there wouldn't be room for the unified toolbar between the icons on the left, and the buttons on the right, in the editor bar. It does so by:

- Reducing the padding between toolbar segments
- Introducing a new breakpoint, xlarge (1080px), that allows the toolbar to stay in the editor bar for longer, before it stacks.
@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Oct 24, 2018
@jasmussen jasmussen added this to the 4.2 milestone Oct 24, 2018
@jasmussen jasmussen self-assigned this Oct 24, 2018
@jasmussen jasmussen requested a review from a team October 24, 2018 09:06
@tofumatt tofumatt requested a review from youknowriad October 24, 2018 12:53
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.

Code-wise it's fine and I, for one, like the new look. 👍 from me. (I think this is possibly fine to sneak in past UI freeze, but that's just me…)

@jasmussen
Copy link
Contributor Author

Thanks for the review! I will wait for thoughts by Riad and Tammie.

@youknowriad
Copy link
Contributor

I personally still prefer the current version because it's more consistent (toolbar vertically separated segments, inspector horizontally separated panels), that said you have my 👍 to move forward, it's just a personal preference.

We should probably update the fullscreen button though

screen shot 2018-10-24 at 15 16 45

@ZebulanStanphill
Copy link
Member

Personally, I think that full-height dividers should be used between different sets of unrelated controls, such as between the fullscreen button and the controls after it, or between the block navigation button and the block toolbar. The shorter lines should be used for dividing within a group of controls, e.g. the different sections of the block toolbar.

@karmatosed
Copy link
Member

I don't think we're there with this iteration but I do think it solves a problem. This makes it a 👍 from me to get it in.

@jasmussen
Copy link
Contributor Author

I don't think we're there with this iteration but I do think it solves a problem. This makes it a 👍 from me to get it in.

Thanks for looking. But just to be clear, it's trivial for me to make it look like https://user-images.githubusercontent.com/1204802/47419612-dab4d200-d77c-11e8-959e-89c4c371f8d1.png if that makes you more happy. I'm not looking to make anyone unhappy here!

@karmatosed
Copy link
Member

karmatosed commented Oct 24, 2018

Visually for me full height seemed to make sense. I would love other opinions though as I do note there is a danger of not thinking they are connected. I also think this menu is something to iterate on after. I am not unhappy either direction.

@chrisvanpatten
Copy link
Contributor

I actually like the shorter version but almost think it should be even shorter 😁 Right now it almost looks like it's a mistake or a rendering bug. A little shorter and it will be more obvious that it's deliberate.

@jasmussen
Copy link
Contributor Author

Given these bits of feedback:

  • Visually for me full height seemed to make sense.
  • I also think this menu is something to iterate on after.
  • Right now it almost looks like it's a mistake or a rendering bug.

... and given we're past UI freeze, I'm going to revert to the full-height style and get this in.

It's always okay to revisit, and as Tammie notes, it likely will be.

@jasmussen
Copy link
Contributor Author

Now it looks like this:

screen shot 2018-10-25 at 09 28 07

Merging if the checks pass.

@jasmussen jasmussen merged commit 4b77281 into master Oct 25, 2018
@jasmussen jasmussen deleted the fix/unified-toolbar-issue branch October 25, 2018 07:45
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
* Fix issue with unified toolbar not fitting

Fixes WordPress#10959.

This PR fixes an issue where at certain viewport widths, there wouldn't be room for the unified toolbar between the icons on the left, and the buttons on the right, in the editor bar. It does so by:

- Reducing the padding between toolbar segments
- Introducing a new breakpoint, xlarge (1080px), that allows the toolbar to stay in the editor bar for longer, before it stacks.

* Revert to fullheight separators.
@mtias mtias added the Mobile Web Viewport sizes for mobile and tablet devices label Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unified toolbar mode hides document settings and general menu
7 participants