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

Editor: Hide toolbar borders dependent on sidebar presence #12004

Merged
merged 6 commits into from
Mar 27, 2017

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Mar 10, 2017

Related: #11536

This pull request seeks to resolve an issue where toolbar borders may be hidden in the editor if the settings panel is closed and between particular medium-size resolutions.

The changes now adjusts for .focus-sidebar, showing borders by default but hiding them if either the screen size is small enough that the editor hits the left and right bounds, accounting for when the settings panel is opened.

Before After
Before After

Testing instructions:

Verify that toolbar borders are present on left and right when post editor settings panel is closed between medium resolution sizes, and present again at larger resolutions.

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 10, 2017
@aduth aduth requested review from mtias and Copons March 10, 2017 14:11
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] S Small sized issue label Mar 10, 2017
@mtias
Copy link
Member

mtias commented Mar 10, 2017

Looking better. There's a moment where I get a horizontal scrollbar and this:

image

The Visual/HTML seems to switch place a couple times during resizing as well.

@Copons
Copy link
Contributor

Copons commented Mar 10, 2017

As for the Visual Toolbar, this change fixes the borders issue, so: 👍 .
It doesn't fix it for the HTML Toolbar and it does not take care of the mode toggler jumping around.

The change for the HTML Toolbar is really easy but there's a thing I'd like to know for sure before suggesting any change (@mtias this is for you!):
the HTML editor previously was width: 100%; max-width: 700px, while right now apparently it stays 100% up to the 960px breakpoint and then shrinks back to 700px.

Now, if this is a wanted change, I'll need to adjust the HTML toolbar width too, and I'd definitely do it in another PR, that I believe could also take care of the mode toggler.

If instead this is an unexpected thing, then, @aduth, it really is a matter of replacing 930px with 720px here: https://github.com/Automattic/wp-calypso/blob/master/client/post-editor/editor-html-toolbar/style.scss#L17 🙂

@mtias
Copy link
Member

mtias commented Mar 10, 2017

There was no intention to change the width of the editor itself. @jasmussen is this something that rings a bell to you?

@jasmussen
Copy link
Member

There was no intention to change the width of the editor itself. @jasmussen is this something that rings a bell to you?

Some toolbar iffiness between the 660 and 960 does ring a bell, and some improvements are noted in #11996. But no, no intention of changing the toolbar width at all.

@Copons
Copy link
Contributor

Copons commented Mar 10, 2017

I'm not talking about the (HTML) toolbar's width, but the content's.

Here's a gif of what's happening to me: https://cloudup.com/cM-93br01f3 (linked because it's 7MB).

I honestly don't even remember how it was before the change, but if I look at how disjointed the content and the toolbar look, it's quite weird.

To be more clear, this is what happens in HTML mode:

<720px
Toolbar: 100%
Content: 100%

720px - 960px
Toolbar: 700px
Content: 100%

>960px
Toolbar: 700px
Content: 700px

Was the content (in HTML mode) 100% wide up to 960px even before?
That I can't remember, but if that's the way we want it, I'll update the HTML Toolbar to be 100% wide too.

Just, it doesn't look right to me for several reasons:

  • The content shrinks back to 700px on >960px.
  • In Visual mode the max width for both toolbar and content is definitely 700px, so it only makes sense to be consistent between the two of them.

@aduth aduth force-pushed the fix/sidebar-toolbar-borders branch from 52e04a4 to 12e7b0e Compare March 17, 2017 18:10
@aduth
Copy link
Contributor Author

aduth commented Mar 17, 2017

Last few commits should clear up the reported issues, I believe.

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

There are some other issues I'm noticing.

HTML Toolbar's z-index is higher than the sidebar's, so it happens that on smallish screens the toolbar covers 1px of the sidebar:

screen shot 2017-03-23 at 18 44 19

This is easily fixable adding a z-index: 2 to the sidebar (with the whole z-index() function).
(Here's the reason why I had to give the HTML toolbar a z-index: 1: #11750)
Or carefully adjusting the toolbar width, maybe, but it sounds incredibly painful.


Again on smallish screens, but this time in both modes, when scrolling down, the sticky toolbars aren't sized correctly. (notice that it's shorter than the width).

screen shot 2017-03-23 at 18 50 27

This seems to be because the toolbar width, when fixed, is calculated as 100% - 273px, but at those screen sizes the sidebar is slightly smaller (229px).

@aduth aduth force-pushed the fix/sidebar-toolbar-borders branch from 5b64db2 to 035c8de Compare March 24, 2017 14:03
@aduth
Copy link
Contributor Author

aduth commented Mar 24, 2017

Nice catches @Copons , thanks! These should be fixed in d99a5ea and 035c8de respectively.

Again on smallish screens, but this time in both modes, when scrolling down, the sticky toolbars aren't sized correctly.

I couldn't reproduce this in Visual mode and found per styles that it should be correctly applying pinned width for mid-size viewports:

https://github.com/Automattic/wp-calypso/blob/e002e8d/client/components/tinymce/style.scss#L52-L55

@matticbot matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Mar 25, 2017
@aduth aduth force-pushed the fix/sidebar-toolbar-borders branch from 035c8de to 7823e02 Compare March 27, 2017 12:28
@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 27, 2017
@aduth aduth merged commit b4a8516 into master Mar 27, 2017
@aduth aduth deleted the fix/sidebar-toolbar-borders branch March 27, 2017 12:36
@matticbot matticbot added the [Size] M Medium sized issue label Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. [Size] M Medium sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants