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

UI: Add a fullscreen mode #9567

Merged
merged 5 commits into from
Sep 5, 2018
Merged

UI: Add a fullscreen mode #9567

merged 5 commits into from
Sep 5, 2018

Conversation

youknowriad
Copy link
Contributor

closes #9334

This PR is the second part of #9334 and adds a fullscreen mode.
The CSS contains some "hacks" to allow switching modes without refreshing the page but I think it's fine.

@youknowriad youknowriad added General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback. labels Sep 3, 2018
@youknowriad youknowriad self-assigned this Sep 3, 2018
@youknowriad youknowriad requested review from karmatosed, mtias, kjellr, jasmussen and a team September 3, 2018 10:30
@@ -272,6 +272,10 @@
margin-left: -18px;
}
}

body.is-fullscreen-mode #{$selector} {
left: 0 !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used important to avoid overriding all the difference use-case above. It just seemed right :)

@kjellr
Copy link
Contributor

kjellr commented Sep 3, 2018

This is looking awesome  — I really like how fullscreen feels.

While using this, I clicked the x and expected it to just turn off full screen. 😄 Considering I should've known what to expect here, that seems like a good indication that we should make that button clearer. Maybe we should use the "close" button in the upper left, instead of the x icon:

I'm interested to hear what others think.

@youknowriad
Copy link
Contributor Author

While using this, I clicked the x and expected it to just turn off full screen. 😄 Considering I should've known what to expect here, that seems like a good indication that we should make that button clearer.

I agree it's not perfect, but the current behavior is very similar to the customizer which is already a familiar fullscreen page for WordPressers

@youknowriad youknowriad closed this Sep 3, 2018
@youknowriad youknowriad reopened this Sep 3, 2018
@youknowriad
Copy link
Contributor Author

Damn buttons :P

@karmatosed
Copy link
Member

This is so good! I am very excited for this and thank you everyone that has worked on this and pushed it along ✨

@jasmussen
Copy link
Contributor

Nice work, this is working better than I would've expected, actually. GIF:

fullscreen

The X for close is working well, and is used in other places as well. It's a bit curious, because it feels very right to palace the X on the left here. But in other places, we place it on the right. Is this an inconsistency we want to look at? If yes, I think I'd prefer using a plain "Close" button rather than moving the X to the right, simply because because "More" menus should always be rightmost as they are to indicate "overflow" of buttons to the left of it, in an LTR reading direction.

I suppose an exception could be made, but I'm not sure if it would look weird with the sidebar close button being stacked below. Though we could decide to hide the sidebar close button on the desktop breakpoint, since when it's a sidebar it's easily toggled using the cog.

The "back to where you were" isn't working quite as well as I thought it would. I think it might work better if it always took you to the same place — like the posts list.

If the "Add new post" button in the toolbar was the primary interface (or only) for creating new posts, "back to where you were" would probably work better.

@youknowriad
Copy link
Contributor Author

The "back to where you were" isn't working quite as well as I thought it would. I think it might work better if it always took you to the same place — like the posts list.

I updated this as suggested.

@@ -105,11 +105,19 @@ body.gutenberg-editor-page {
bottom: 0;
left: 0;
min-height: calc(100vh - #{ $admin-bar-height-big });

body.is-fullscreen-mode & {
min-height: calc(100vh);
Copy link
Contributor

Choose a reason for hiding this comment

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

is calc() necessary in this case?

}

// The WP header height changes at this breakpoint
@include break-medium {
min-height: calc(100vh - #{ $admin-bar-height });

body.is-fullscreen-mode & {
min-height: calc(100vh);
Copy link
Contributor

Choose a reason for hiding this comment

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

is calc() necessary in this case?

<IconButton
icon="no-alt"
href={ addQueryArgs( 'edit.php', { post_type: postType } ) }
label={ __( 'Close' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this button is a link, and communicated as such to assistive technologies users, I'd rather consider to use a label that communicates the link destination instead of an action like "Close", which is more appropriate for buttons. RIght now, the link navigates back to the posts list, and it should communicate that 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used the same label as the customizer originally but yeah it's fine to update. I guess something like Back to post list or something would work?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would work for me 👍 Yeah... I know about the Customizer link 🙈

@afercia
Copy link
Contributor

afercia commented Sep 4, 2018

Nice job, also from an a11y perspective (but please see the comment about the "Close" link):

  • the main H1 is preserved, and the headings hierarchy is correct
  • the ARIA landmarks are preserved
  • the Ctrl+backtick shortcut to navigate the main regions works
  • the Tab sequence makes sense and there are no hidden tab stops

🎉

@afercia
Copy link
Contributor

afercia commented Sep 4, 2018

And with the sidebar closed, it looks really nice! Visually, loving it 😍

screen shot 2018-09-04 at 18 30 15

Have you considered to add a keyboard shortcut to toggle fullscreen mode? (yeah, new challenge to find a non-conflicting shortcut!)

@youknowriad
Copy link
Contributor Author

I decided to use the view_items post type label for this button as it seemed the most appropriate label.

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.

Technically this seems to be working very solidly.

There seems to be wide agreement among the reviewers here that this has value and should be tested. I happen to agree with that.

It also adds value to laptops that have limited screen real estate, like 11 inch laptops, presumably tablets as well. As such, I think we should get this in and iterate.

@youknowriad youknowriad merged commit 93f1f22 into master Sep 5, 2018
@youknowriad youknowriad deleted the add/fullscreen-mode branch September 5, 2018 10:18
@mtias
Copy link
Member

mtias commented Sep 6, 2018

Love this, thanks for the quick implementation!

@ktmn
Copy link

ktmn commented Sep 6, 2018

This is the best thing ever, thanks for making it!

@SNaushadS
Copy link

Thank you for all who worked on getting this done, I needed it badly.

I have some feedback on exiting/closing the fullscreen mode.

TLDR: X at the top left gave the affordance that i can get Out of the Full Screen mode. It should either close the mode or be removed.

What did i do:

  1. Clicked on the hamburger menu.
  2. Clicked on Full Screen mode.
    Full Screen Mode was now active, Awesome. It’s spacious and helps me focus on writing.
    Then i wanted to get Out of the fullscreen mode. So i tried to find the way out.
  3. Found the X at the top left and clicked on it.

My expectation: It should’ve got me out of fullscreen mode.
4. Instead i was provided warning by the browser if i want to Stay or Leave. Doesn’t match with user expectation.
as already observed by @kjellr
5. Then i tried to find out the “designed” way to close/exit Fullscreen Mode
fullscreengutenberg
Also, the mouse travel from left edge to right edge is a bit painful.
6. Fullscreen had a right tick next to it, clicked it and i was out of the fullscreen mode.

Comments

  • Fullscreen behavior is typically that of a Modal.
  • Hence the general expectation would be that the X either on the top-right or top-left would close/exit/take user out of the Mode.

Suggestions:
Let

  • The X could either allow user to exit the fullscreen mode (as per general interaction expectations)
  • Or be removed and let user exit the mode only from hamburger menu by unticking.

Let me know how further i can help.

kjellr added a commit that referenced this pull request Sep 12, 2018
As per comments in #9567. The `x` icon to close the editor while in fullscreen mode could be misconstrued as a button for exiting out of full screen mode. Since the button actually takes you back to the All Posts screen, a back arrow may make more sense here.
@aduth
Copy link
Member

aduth commented Sep 12, 2018

To @SNaushadS 's feedback, there's an iteration being proposed / discussed in #9838

@ktmn
Copy link

ktmn commented Sep 14, 2018

Looks like wp.components.Modal has a bug in fullscreen mode.

Above is the modal in normal mode. Below is the modal in fullscreen mode and the CSS rule causing it.

@jasmussen
Copy link
Contributor

@ktmn good catch. I will take a look — I need to look at the fullscreen mode regardless as when an editor style applies a colored background, a bottom padding offsets this.

@jasmussen
Copy link
Contributor

The issue you reported, @ktmn, will be fixed either by #9973 or #9900. They both address it.

kjellr added a commit that referenced this pull request Sep 19, 2018
As per comments in #9567. The `x` icon to close the editor while in fullscreen mode could be misconstrued as a button for closing full screen mode. Since the button actually takes you back to the All Posts screen, an exit icon makes more sense here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform "Fix Toolbar to Top" into "Focus Mode"
9 participants