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: Desktop app style layout #2131

Merged
merged 9 commits into from
Jan 11, 2016
Merged

Editor: Desktop app style layout #2131

merged 9 commits into from
Jan 11, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jan 5, 2016

This pull request seeks to implement a redesigned post editor layout, matching the latest sidebar layout revisions made to the rest of Calypso.

Before After
Before After

Design notes:

Further revisions to the mobile layout are planned, and are not included in the changes here. The mobile layout should continue to display as it does currently without issues.

Testing instructions:

This is a significant set of changes that touches many areas of the editor page, and as such there are potentially a number of edge case behaviors that have not been accounted for. Please thoroughly test all common editor workflows!

Particularly, confirm that the following items do not have layout issues in all supported browsers:

  • Notices (post publish, email verification)
  • Standard and non-standard viewport sizes (including mobile actions sidebar)
  • Header featured image
  • Visual / HTML edit mode toggling
  • Editor toolbar pinning, advanced toggle

@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. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Jan 5, 2016
@aduth aduth self-assigned this Jan 5, 2016
@nylen
Copy link
Contributor

nylen commented Jan 5, 2016

Whoa, this is great!

Things about this that make me really happy:

  • Toolbar pinning rewritten! (Seems to work really well now, too)
  • Cleaned up some nasty chains of padding + margin + negative margin, and other CSS cruft!
  • No more weird ground control pinning thing - now the sidebar scrolls separately!
  • Fixed-position word count!

I'll keep testing this today and tomorrow, but I noticed a few minor issues right away:

(1) I'm not really sure if anything can be done about this, but all the white space to the left and right of the toolbar looks a bit weird to me on wider windows:

(2) Instead of the current light blue-gray color, I think we should make the space below TinyMCE (if any) white as well:

(3) At <960px the toolbar has a long-content-fade applied. Strangely, this disappears when I scroll the editor content over it:

In master, this is more "everything related to toolbar pinning is subtly broken" than an existing issue. This PR is such a huge improvement to toolbar pinning that I've mentioned it twice now :)

@nylen
Copy link
Contributor

nylen commented Jan 5, 2016

I just pushed a few commits to the word count:

Better match @kellychoffman's original design from #300

This keeps the word count readable if there is post text under it.

Before:
image image

After:
image

Add singular + plural translation of count string

Per the i18n docs. Also fixes previous "1 words" text in English.

Remove hover effect

Clicking the word count doesn't do anything so it shouldn't have a hover effect.

max-width: 220px;
margin-right: 24px;
@include breakpoint( ">660px" ) {
width: 700px;
Copy link
Member

Choose a reason for hiding this comment

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

This is getting applied to the toolbar as well, which creates this gap:

screen shot 2016-01-05 at 5 47 53 pm

Added 63dff24 that seems to fix it, but there could be a better way?

@folletto
Copy link
Contributor

folletto commented Jan 6, 2016

(1) I'm not really sure if anything can be done about this, but all the white space to the left and right of the toolbar looks a bit weird to me on wider windows:

True, this isn't ideal, but let's take it as incremental step. We did a lot of design iterations trying to sort that out, but there are a fair amount of constraints there, so I'd rather start with this and pull in the full-width view, and then we'll iterate on that bit. :)

(2) Instead of the current light blue-gray color, I think we should make the space below TinyMCE (if any) white as well:

The idea is that the right part is always, fully, white. It simplifies the look (making it gray would create a discontinuity and add more visual change on the page) and makes it more immersive.

However, what we really have to do is to avoid the text to stick to the end of the page all the time. All the good text editors make sure that you never end up typing against the edge of the screen, but always mid-way up (from 1/2 to 2/3 from the top):

writing-point

See a rough demo here.

I'd personally suggest 2/3 or 3/4 options in general, since 1/2 could be too jarring if you're editing (or, it never turns on when you edit, just when you are at the end of the document and start typing).

I'm not sure however if that's something we should do in this PR since it requires some extra logic, but at least I would add a bottom white margin always present when the vertical scrollbar is visible.

This keeps the word count readable if there is post text under it.

Agreed on the white partial opacity background, but I wouldn't border it. In the most common screen sizes there wouldn't be overlap... and the problem would disappear entirely with the added border or 2/3 logic as I mentioned above. :)

Further revisions to the mobile layout are planned, and are not included in the changes here.

Question in terms of logistics: since this PR already works well (i.e. no regressions) at mobile size, maybe we can consider do merge this in first, and then do a different one for the mobile changes, instead of a single PR doing both? :)

Overall: GREAT JOB. 💯

Tested on Chrome, Firefox, Safari.

Bugs/notes:

(1) On Firefox the editor tabs are layered on top of the sidebar in mobile view:

screen shot 2016-01-06 at 10 41 40

(2) There's a double-scroll effect when the sidebar is open on mobile. This wasn't present before since we didn't have two independently scrollable areas.

(3) At mobile size the "Schedule" dropdown goes slightly out of the right edge of the screen (and adds a horizontal scrollbar).

@aduth
Copy link
Contributor Author

aduth commented Jan 6, 2016

Thanks for the great feedback!

Things about this that make me really happy:

Glad you noticed these things. All of which made me similarly happy!

all the white space to the left and right of the toolbar looks a bit weird to me on wider windows:

One approach I had tried was to extend the border between the two toolbars to the edges of the editable area. The implementation is very hacky (::before, ::after pseudo-elements with absolute positioning) and it really doesn't look aesthetically great. As @folletto mentioned, we can iterate on this.

I think we should make the space below TinyMCE (if any) white as well:

Yep, this was unintentional. Originally I had the white background on .post-editor, which covers the entire page, though found that this had affected the background of the mobile action bar. I'll see what I can to do to accommodate both.

At <960px the toolbar has a long-content-fade applied

I'll take a look. Probably related to fixed positioning of the pinned toolbar.

Add singular + plural translation of count string

D'oh! I should know better 😄

Remove hover effect

I'm fine with removing the hover effect, though generally speaking not sure I agree that only actionable items deserve hover effects.

This is getting applied to the toolbar as well, which creates this gap: Added 63dff24 that seems to fix it, but there could be a better way?

Might just need to change the original breakpoint to >960px, or whatever matches the stacking-inline toolbar restyling. I'll take a look.

However, what we really have to do is to avoid the text to stick to the end of the page all the time.

@folletto, can you link to some resources which explain why this is desirable? I don't really see the point, and it seems like a waste of available screen real estate. In any case, I'd agree we can tackle this separately.

Agreed on the white partial opacity background, but I wouldn't border it.

👍 to no border. I quite like how it doesn't draw too much attention when no border is applied.

since this PR already works well (i.e. no regressions) at mobile size, maybe we can consider do merge this in first, and then do a different one for the mobile changes, instead of a single PR doing both? :)

Sorry, failure to properly communicate 😄 Said planned revisions were always intended for a separate pull request so as to keep this one limited in scope.

On Firefox the editor tabs are layered on top of the sidebar in mobile view:

Will look to fix.

There's a double-scroll effect when the sidebar is open on mobile. This wasn't present before since we didn't have two independently scrollable areas.

Do you see this on other Calypso pages as well? Almost all styles for the sidebar are now inherited from the Calypso .sidebar class.

@folletto
Copy link
Contributor

folletto commented Jan 6, 2016

Do you see this on other Calypso pages as well? Almost all styles for the sidebar are now inherited from the Calypso .sidebar class.

Good point, I didn't check. Happens there too. Not a regression. :)

@aduth
Copy link
Contributor Author

aduth commented Jan 6, 2016

I just pushed a number of changes that seek to resolve the issues noted here:

  • Apply white background to entire editor page (a712886)
  • Remove border from word count (5571dc3)
  • Set toolbar fade as fixed when editor is pinned (d671390)
  • Fix toolbar gap in mid-range viewports (596c892)
  • Prevent mode switch from appearing above sidebar (8da769a)

At mobile size the "Schedule" dropdown goes slightly out of the right edge of the screen (and adds a horizontal scrollbar).

At which resolution are you seeing this? I've tried at as low a resolution as 320px wide, but not encountering this issue. I don't think we need to accommodate lower resolutions.

@folletto
Copy link
Contributor

folletto commented Jan 6, 2016

At which resolution are you seeing this? I've tried at as low a resolution as 320px wide, but not encountering this issue. I don't think we need to accommodate lower resolutions.

screen shot 2016-01-06 at 18 43 21

Scrollbars I guess, then. :)

@folletto
Copy link
Contributor

folletto commented Jan 6, 2016

Everything else looks good (Chrome, Firefox, Safari). 💯

@kellychoffman
Copy link
Member

At mobile size the "Schedule" dropdown goes slightly out of the right edge of the screen

At <960px, the entire sidebar is getting cut off on the right side, which I think is the cause of this.

screen shot 2016-01-06 at 6 23 25 pm

@aduth
Copy link
Contributor Author

aduth commented Jan 7, 2016

At <960px, the entire sidebar is getting cut off on the right side, which I think is the cause of this.

Do you mean <660px?

I imagine this is also common to all sidebars in Calypso, probably due to a combination of position: fixed and width: 100vw. Solution might be to use percentage widths, or at least percentage as a max-width? In either case, it doesn't seem like this is specific to the editor and most viewports of this size don't have scrollbars anyways.

(When I say above that almost all styles are inherited from the base sidebar component, I should clarify that any and all styles are inherited 😄 )

@folletto
Copy link
Contributor

folletto commented Jan 7, 2016

In either case, it doesn't seem like this is specific to the editor and most viewports of this size don't have scrollbars anyways.

It's a marginal case, but the desktop app can be kept at that size. ;)

FWIW, I think it's because until the sidebar gets a scrollbar, it's the main window scrollbar present and that makes the other layer to fall behind. You can notice it if you turn on scrollbars, and then make the sidebar longer than the screen or shorter. It's a mix of issues. :)

However I agree it's not a blocker.

@kellychoffman
Copy link
Member

Yep, I meant 660px.

I bring it up because its not happening in production. It also corrects itself when a metabox is expanded, which unfortunately causes an undesirable jump:

jumps

@nylen
Copy link
Contributor

nylen commented Jan 8, 2016

In master the area beside the header featured image is white. Now it's blue/gray. Was that intentional?

Before After
image image

I've been testing this out some more and haven't found any other issues. Apart from the minor issues others have noted, this is looking really good 👍

@folletto
Copy link
Contributor

folletto commented Jan 8, 2016

I bring it up because its not happening in production. It also corrects itself when a metabox is expanded, which unfortunately causes an undesirable jump:

I think it's the same issue I mentioned above. Unfortunately that's already in production, if you check the other sections that are using the new full-width layout:

licecap-

It's just less noticeable.

I'd suggest to address it separately from this PR. :)

@kellychoffman
Copy link
Member

Interesting. Not seeing what you're seeing, but it would be fine to address in a separate PR.

@kellychoffman
Copy link
Member

In master the area beside the header featured image is white. Now it's blue/gray. Was that intentional?

Yes, its supposed to be consistent with Reader. I made some updates to that area in d96e0a4.

@mtias
Copy link
Member

mtias commented Jan 11, 2016

Seems like the accordions should match the sidebar background.

@aduth
Copy link
Contributor Author

aduth commented Jan 11, 2016

Seems like the accordions should match the sidebar background.

Yeah, that's how it is in the concepts, so I must have missed it. Changed in 2152d12.

@mtias
Copy link
Member

mtias commented Jan 11, 2016

Nice work @aduth :)

@aduth aduth force-pushed the update/editor-app-style branch from 5293cd5 to dbb549e Compare January 11, 2016 18:29
@apeatling
Copy link
Member

Looking really really good, feels much more like part of the app as a whole now.

One nitpick, which I think could help is matching the look and space above the site box in the sidebar when switching between the editor and my sites:

size

Notice how in the editor the space above is larger, and the back arrow with text doesn't match. Can we match those up? I think those little things can make a difference.

@aduth
Copy link
Contributor Author

aduth commented Jan 11, 2016

One nitpick, which I think could help is matching the look and space above the site box in the sidebar when switching between the editor and my sites:

IANAD (I'm not a designer), but I kinda wish the Switch Site button in the My Sites page was similarly padded as the All Posts editor link in this branch. It has the more padding at smaller resolutions, but feels arbitrarily narrow in the desktop view (even compared to other navigation links). Thoughts?

@aduth
Copy link
Contributor Author

aduth commented Jan 11, 2016

I can narrow the editor link padding for the time-being and we can tackle the My Sites sidebar separately.

@mtias
Copy link
Member

mtias commented Jan 11, 2016

The idea was to make the main sidebar button match the editor. I'll open a PR to actually use Button compact borderless in the My Sites sidebar and we can loop back here.

@aduth
Copy link
Contributor Author

aduth commented Jan 11, 2016

Ok, I'm operating under the assumption that we'll move forward with the changes we have here, then separately revise the My Sites sidebar to match the styling of the editor. @mtias also mentioned a desire to update all use-cases to incorporate the borderless button component.

@apeatling , sound okay to you?

@aduth aduth force-pushed the update/editor-app-style branch from 196b8cc to a5be42c Compare January 11, 2016 19:32
@apeatling
Copy link
Member

@aduth Sounds good, as long as it's on the radar to update.

@mtias
Copy link
Member

mtias commented Jan 11, 2016

@apeatling opened #2286. And will improve the editor one once it lands.

@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 Jan 11, 2016
aduth added a commit that referenced this pull request Jan 11, 2016
@aduth aduth merged commit 40e6909 into master Jan 11, 2016
@aduth aduth deleted the update/editor-app-style branch January 11, 2016 19:42
@lancewillett lancewillett removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 19, 2016
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants