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

Fullscreen Mode: Change the "Back" button to toggle the sidebar #21121

Merged
merged 24 commits into from
Apr 22, 2020

Conversation

shaunandrews
Copy link
Contributor

With the recent move to make Fullscreen mode the default, I've heard from many people that they expect to be able to toggle the WordPress sidebar menu by clicking on the WordPress logo in the top left. This PR tries that:

Fullscreen Sidebar Toggle i1

@shaunandrews shaunandrews added the Needs Design Feedback Needs general design feedback. label Mar 24, 2020
@github-actions
Copy link

github-actions bot commented Mar 24, 2020

Size Change: +2.65 kB (0%)

Total Size: 845 kB

Filename Size Change
build/annotations/index.js 3.62 kB -1 B
build/block-directory/index.js 6.24 kB -1 B
build/block-editor/index.js 105 kB +1 B
build/block-editor/style-rtl.css 10.1 kB -21 B (0%)
build/block-editor/style.css 10.1 kB -22 B (0%)
build/block-library/editor-rtl.css 7.13 kB +33 B (0%)
build/block-library/editor.css 7.13 kB +30 B (0%)
build/block-library/index.js 112 kB +157 B (0%)
build/block-library/style-rtl.css 7.19 kB +15 B (0%)
build/block-library/style.css 7.19 kB +14 B (0%)
build/blocks/index.js 57.7 kB -1 B
build/components/index.js 198 kB +1 B
build/data-controls/index.js 1.25 kB +1 B
build/edit-post/index.js 28.2 kB +252 B (0%)
build/edit-post/style-rtl.css 12.5 kB +141 B (1%)
build/edit-post/style.css 12.5 kB +141 B (1%)
build/edit-site/index.js 10.6 kB +103 B (0%)
build/edit-site/style-rtl.css 5.2 kB +156 B (3%)
build/edit-site/style.css 5.2 kB +155 B (2%)
build/edit-widgets/index.js 8.33 kB +838 B (10%) ⚠️
build/edit-widgets/style-rtl.css 5 kB +339 B (6%) 🔍
build/edit-widgets/style.css 5 kB +342 B (6%) 🔍
build/editor/style-rtl.css 3.27 kB -10 B (0%)
build/editor/style.css 3.27 kB -10 B (0%)
build/keyboard-shortcuts/index.js 2.51 kB -1 B
build/list-reusable-blocks/index.js 3.12 kB -1 B
build/media-utils/index.js 5.29 kB -1 B
build/rich-text/index.js 14.8 kB -1 B
build/server-side-render/index.js 2.67 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@MichaelArestad
Copy link
Contributor

I want this and I want it now! <3

@mapk
Copy link
Contributor

mapk commented Mar 25, 2020

Moving this functionality to the top level is great. While it doesn't provide the top bar, it's still usable and surfaces the wp-admin navigation with ONE click. Great exploration!

@SchneiderSam
Copy link

This is soooooo cool.

@paaljoachim
Copy link
Contributor

paaljoachim commented Mar 25, 2020

This is awesome! It sure makes going into Full Screen mode a lot easier.
For anyone who wants to test this out. Go to gutenberg.run and insert 21121 (PR number).

Along with this PR let's also get a shortcut in place. #20626

@shaunandrews
Copy link
Contributor Author

A couple of things I could use help with:

  • I think when you toggle the sidebar open, the keyboard :focus should jump to the wp-admin sidebar.
  • Right now, the sidebar sticks around until you click the WP logo again. Part of me thinks that clicking anywhere outside of the sidebar should hide the sidebar again. In fullscreen-mode, I don't think the sidebar should stick around.

I'm not sure how to go about implementing either of the above, but would love to learn. All tips, tricks, and commits welcomed.

@shaunandrews shaunandrews requested a review from mtias March 26, 2020 02:47
@paaljoachim
Copy link
Contributor

Testing out icons for bringing back the sidebar menu.

Original.
Sidebar-W-original

Grey W.
Sidebar-W-Gutenberg-5

White W.
Sidebar-W-Gutenberg-7

Grey W with vertical line.
Sidebar-grey-W-with-line

Grey W with dashes and vertical line.
Sidebar-W-Gutenberg-1

White W with dashes and vertical line.
W-show-sidebar-Gutenberg-2

White W with dashes and vertical line 2.
Show-sidebar-W-Gutenberg-3

Grey W with horizontal dashes.
Sidebar-W-Gutenberg-4

@bph
Copy link
Contributor

bph commented Mar 26, 2020

This is an awesome idea, and well executed. Thank you @shaunandrews :-) Sorry, I can't help you with development but I wanted you to know that a lot of end users will be quite happy with this.

@mtias
Copy link
Member

mtias commented Mar 28, 2020

Thanks for exploring this @shaunandrews. We should look at the centered logo a bit in the open state: it'd make sense to see if we can render the site title when it expands (and keep it hidden when collapsed) to balance it better.

@ItsJonQ
Copy link

ItsJonQ commented Mar 31, 2020

A couple of things I could use help with:

  • I think when you toggle the sidebar open, the keyboard :focus should jump to the wp-admin sidebar.

  • Right now, the sidebar sticks around until you click the WP logo again. Part of me thinks that clicking anywhere outside of the sidebar should hide the sidebar again. In fullscreen-mode, I don't think the sidebar should stick around.

@shaunandrews Haii!! Great work on this update! I can certainly help you with these items :)

re: Focus jump:

To do this, we'll need to hook into some sort of onOpen state transition to make the focus. I think maybe keeping the focus on the "W" logo may be better. However, perhaps when...

  • Focus is on "W" logo, and...
  • Menu is open, and...
  • User presses DOWN on the keyboard,...
  • :focus jumps to the first menu item

re: Closing menu when clicking outside

This is often accomplished using 2 techniques:

  1. "Overlay"

When the menu is open, an "Overlay" (a transparent div) is rendered on top of the Editor, example:

overlay

This blocks interaction for anything underneath it, and allows you to easily attach an onClick handler on it. Which means, when you click on the overlay, the menu can close.

  1. "Clicking outside"

This is the trickier, but often more preferrable, way to handle these interactions. What needs to be done is a click event listener needs to be added, and logic needs to read something like:

  • If the (clicked) event.target is...
  • NOT the "W" logo, and...
  • NOT the menu, and...
  • NOT the top toolbar,....
  • Then close.

Hope this helps 🙏


Edit: Last thought! What do you think about closing when ESC is pressed as well?

@mapk
Copy link
Contributor

mapk commented Mar 31, 2020

This is beautiful!
I'm in favor of seeing a prototype that works in Matias' suggestion. Keep the logo fixed to the left and add the site title when expanded.

This is also the desire after today's triage meeting in slack.

@ItsJonQ
Copy link

ItsJonQ commented Apr 1, 2020

Haiii!! I coordinated with @shaunandrews .
I'm gonna try to help out a bit with the JS code side.

🤞

@ItsJonQ
Copy link

ItsJonQ commented Apr 1, 2020

Heya @shaunandrews !

I have updates :). I've made some refactors and enhancements in a separate branch:
https://github.com/WordPress/gutenberg/tree/try/fullscreen-sidebar-toggle-refactor

(This branch was also rebased with latest master)

You can see my changes here:
9a9f9ef

(Because it's been rebased with master, some things around npm install may need to be done)

TLDR:

  • Create a custom React hook to better manage the open/close menu state
  • Leveraged hook to bind the click + keyboard events for closing or (re)focusing

Let me know what you think!

If these changes are okay, I can rebase it into this branch.


Note: I haven't done anything with Site Title yet.

it'd make sense to see if we can render the site title when it expands (and keep it hidden when collapsed)

Keep the logo fixed to the left and add the site title when expanded.

We can do this. However, I'm not too sure what it may look like if the site title was longer. In the regular WP Admin, the "W" logo and title are separate from the menu, allowing longer titles to flow.

Example:

Screen Shot 2020-04-01 at 10 47 18 AM

I think a mockup of this would be helpful 😊

Thank you!

@shaunandrews shaunandrews added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Apr 1, 2020
@ItsJonQ ItsJonQ force-pushed the try/fullscreen-sidebar-toggle branch from adb8046 to 960b204 Compare April 1, 2020 16:39
@shaunandrews
Copy link
Contributor Author

Let me know what you think!

Super cool, @ItsJonQ — thanks for your help on this. I'd love to see your branch merged into this one.

One thing I'm still unsure about is the keyboard interaction. Having to press the down arrow to start navigating the wp-admin sidebar seems strange. If you look at how the inserter works, when you tab to it and select, the :focus goes into the popover. I feel like this maps to the wp-admin toggle; Selecting the logo with the keyboard seems like it should move the :focus into the menu.

--

As for the site title, I like the idea in theory. But I'm not sure there's enough room for it with the current width of the menu. Here's what it would look like with a truncated site title:

image

We could change the width of the menu (only within the Editor context) to add some overall space, but I'd expect that to feel strange in use:

image

We could break the title down to its own line:

image

@ItsJonQ
Copy link

ItsJonQ commented Apr 1, 2020

@shaunandrews Thanks for 🍐 'ing with me on this one!

I just rebased my changes from my refactor branch onto yours and pushed it up.

One thing we noticed was that the name of FullscreenModeClose no longer described what the component did.

I renamed it to AdminMenuToggle, and have adjusted CSS classNames and E2E tests accordingly.

@ItsJonQ
Copy link

ItsJonQ commented Apr 1, 2020

Selecting the logo with the keyboard seems like it should move the :focus into the menu.

@shaunandrews Ah yes, the keyboard interactions are tricky for this one. I just pushed an update that focuses using TAB rather than DOWN.

Let me know how this feels :)

@enriquesanchez
Copy link
Contributor

Keyboard interaction feels good to me, but I found one small issue.

If I move focus to the "W" and hit Enter, the wp-admin menu expands and my focus stays on the "W". This is expected. I can then move focus to the first item in the menu by hitting TAB.

The one problem I found was going back. If try to do shift + TAB to go back up to the "W" button, I can't seem to be able to land focus on it. I do see a "Skip to toolbar" skip link, which then takes me to the "W" button if I do one more TAB, but that was unexpected. I expected to be able to go back the same way I came down.

If I use the "Skip to toolbar" link, I'm not really taken to the "toolbar" because if I do a TAB again after focusing on the "W", my focus moves down to the "Dashboard" in the menu. That is unexpected, I assumed that because I'm now on the toolbar I would move focus to the inserter button instead.

I would need to collapse the menu first, so that I'm now on the toolbar and can move to the inserter.

@shaunandrews
Copy link
Contributor Author

shaunandrews commented Apr 1, 2020

This overlaps the top toolbar:

image

@timhibberd
Copy link

@shaunandrews I like this a lot but this would tie a navigation function to an icon that a lot of MSPs hide for business sites. So the question is...if this is hidden can we be assured of no side-effects? Will the icon that inherits that position now inherit the navigation logic?

If you are going to do this, anchor it as a mandatory navigation element with a php API to replace it with a different icon / function. Even that would break on legacy sites who don't know about the new API.

@shaunandrews
Copy link
Contributor Author

Rebased and fixed the display bug in Safari.

@shaunandrews shaunandrews merged commit ac7731d into master Apr 22, 2020
@shaunandrews shaunandrews deleted the try/fullscreen-sidebar-toggle branch April 22, 2020 18:10
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 22, 2020
@jasmussen
Copy link
Contributor

The left menu doesn't scroll when in fullscreen mode on a small viewport:

Screenshot 2020-04-24 at 14 58 32

The "fix" in non-fullscreen mode is to allow the HTML element to scroll, causing a double scrollbar on the right. Not an ideal fix, and would love ideas for making that better.

@mtias
Copy link
Member

mtias commented Apr 27, 2020

I think we need to revert this one and continue working on it for a bit. There are aspects that need to be better handled for an adequate integration, otherwise states like this are possible:

image

@aduth
Copy link
Member

aduth commented Apr 27, 2020

Revert pull request at #21929

aduth added a commit that referenced this pull request Apr 27, 2020
ItsJonQ pushed a commit that referenced this pull request Apr 30, 2020
shaunandrews pushed a commit that referenced this pull request May 8, 2020
shaunandrews pushed a commit that referenced this pull request May 15, 2020
ItsJonQ pushed a commit that referenced this pull request May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.