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.

Sign up for GitHub

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

Jump to bottom

Site Editor: Update Navigation Panel Toggle UI #25622

Merged
merged 8 commits into from
Sep 29, 2020
Merged

Site Editor: Update Navigation Panel Toggle UI #25622

merged 8 commits into from
Sep 29, 2020

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Sep 24, 2020 •
edited
Loading

Description

Update the toggle UI and the back navigation of the Site Editor navigation panel.

See: #23939

How has this been tested?

Tested in the local Gutenberg wp-env with the FSE experiment enabled, in the Site Editor.

Screenshots

Post Editor (for reference) Site Editor (collapsed) Site Editor (expanded)
Screenshot 2020-09-24 at 16 13 49 Screenshot 2020-09-24 at 16 14 24 Screenshot 2020-09-24 at 16 14 37

Types of changes

New feature (non-breaking change which adds functionality

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@Copons Copons added Needs Design Feedback Needs general design feedback. [Feature] Full Site Editing [Feature] Navigation Component A navigational waterfall component for hierarchy of items. labels Sep 24, 2020
@Copons Copons requested a review from youknowriad as a code owner September 24, 2020 14:21
@Copons Copons self-assigned this Sep 24, 2020
@Copons Copons changed the title Update/site editor sidebar back navigation Site Editor: Update Navigation Panel Toggle UI Sep 24, 2020
@github-actions
Copy link

github-actions bot commented Sep 24, 2020 •
edited
Loading

Size Change: +1.02 kB (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-directory/index.js 8.53 kB +1 B
build/block-editor/index.js 128 kB +85 B (0%)
build/block-library/editor-rtl.css 8.57 kB -21 B (0%)
build/block-library/editor.css 8.58 kB -21 B (0%)
build/block-library/index.js 135 kB -296 B (0%)
build/blocks/index.js 47.5 kB -2 B (0%)
build/components/index.js 167 kB -4 B (0%)
build/components/style-rtl.css 15.5 kB +7 B (0%)
build/components/style.css 15.5 kB +8 B (0%)
build/core-data/index.js 12 kB -1 B
build/data-controls/index.js 1.27 kB +2 B (0%)
build/data/index.js 8.43 kB +5 B (0%)
build/edit-navigation/index.js 10.7 kB +239 B (2%)
build/edit-post/index.js 306 kB +1 B
build/edit-post/style-rtl.css 6.25 kB +10 B (0%)
build/edit-post/style.css 6.24 kB +10 B (0%)
build/edit-site/index.js 20.5 kB +197 B (0%)
build/edit-site/style-rtl.css 3.74 kB +234 B (6%) 🔍
build/edit-site/style.css 3.74 kB +236 B (6%) 🔍
build/edit-widgets/index.js 17.9 kB +316 B (1%)
build/edit-widgets/style-rtl.css 2.82 kB +24 B (0%)
build/edit-widgets/style.css 2.82 kB +24 B (0%)
build/editor/index.js 45.4 kB -69 B (0%)
build/editor/style-rtl.css 3.83 kB +26 B (0%)
build/editor/style.css 3.82 kB +24 B (0%)
build/element/index.js 4.44 kB -8 B (0%)
build/rich-text/index.js 13.7 kB -11 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/style-rtl.css 7.61 kB 0 B
build/block-library/style.css 7.6 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.55 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/server-side-render/index.js 2.61 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@Copons Copons mentioned this pull request Sep 24, 2020
Merged
8 tasks
shaunandrews
shaunandrews approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@shaunandrews shaunandrews left a comment

Choose a reason for hiding this comment

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

I think this is a good start; I'm able to open and close the sidebar as expected. I think there's two things could be improved in further followups:

  • There's no way to move keyboard focus from the buttonIcon into the contents of the sidebar. I would expect to be able to use tab or my arrow keys to move down — or, perhaps, it would make sense to move focus into the sidebar, similar to how a dropdown works.
  • I'd love to see a transition with the sidebar animating into view.

@Copons
Copy link
Contributor Author

Copons commented Sep 25, 2020

There's no way to move keyboard focus from the buttonIcon into the contents of the sidebar. I would expect to be able to use tab or my arrow keys to move down — or, perhaps, it would make sense to move focus into the sidebar, similar to how a dropdown works.

@shaunandrews I've updated it to automatically focus the the sidebar on open.
There are 2 gotchas:

  • The first item (which is the one that gets the focus) of the sidebar is the "Back to Dashboard" button.
    I think it makes sense, and also helps keeping a bit of consistency with the post editor back button.

  • Shift+Tab from the back button doesn't focus the "Toggle Navigation" button, but the last element of the header toolbar.
    This is consistent with the Inserter's behaviour, and it makes sense technically (both navigation and inserter are "physically" rendered at the end of the header), but not so much if one wanted to easily close the sidebar after opening it.

vindl
vindl approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@vindl vindl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This works well for me!

I have two possible items for next iterations:

The first item (which is the one that gets the focus) of the sidebar is the "Back to Dashboard" button. I think it makes sense, and also helps keeping a bit of consistency with the post editor back button.

  1. I find this a bit unexpected. Another option might be to activate/focus the template that's currently visible in site editor. Depending on how we present the hierarchy that might also be weird since it can be in nested levels. But at least with current setup it should work fine - we load homepage by default and I'd expect to see its template at the root level.

  2. The animation of toggle and sidebar slide in are a bit out of sync. It's not that noticeable because of short duration. If we want to match them perfectly we could switch to linear animation, since I don't think we can perfectly match cubic bezier ones in general.

@shaunandrews
Copy link
Contributor

The first item (which is the one that gets the focus) of the sidebar is the "Back to Dashboard" button.
I think it makes sense, and also helps keeping a bit of consistency with the post editor back button.

Unlike @vindl, I think this feels good and is the behavior I'd expect.

Shift+Tab from the back button doesn't focus the "Toggle Navigation" button, but the last element of the header toolbar. [...] both navigation and inserter are "physically" rendered at the end of the header...

Now that you mention it, I find it strange that the navigation-panel is being rendered inside the skeleton__body; Wouldn't it make more sense to render it above the skeleton__header, as the first element within the the interface-skeleton? Taking this even further, perhaps the navigation-toggle__button doesn't belong in the skeleton__header either. I think moving these components outside the header and body could help with both the transition and an alignment issue that has popped up in #25630.

This is consistent with the Inserter's behaviour

I believe this is a bug with the Inserter sidebar in the Site Editor; In the Post Editor the keyboard focus is "trapped" within the inserter, similar to a modal. See #24429 for more info. I believe we should fix that in a separate PR, as its unrelated to this one.

That said, I don't think this sidebar should act like a popover. If it did that would mean navigating away from the sidebar would dismiss it.

The animation of toggle and sidebar slide in are a bit out of sync. It's not that noticeable because of short duration. If we want to match them perfectly we could switch to linear animation, since I don't think we can perfectly match cubic bezier ones in general.

I think linear could be better if it means we can get rid of the staggered transitions.

@vindl vindl merged commit 4c04c6e into master Sep 29, 2020
@vindl vindl deleted the update/site-editor-sidebar-back-navigation branch September 29, 2020 13:25
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Sep 29, 2020
@vindl vindl mentioned this pull request Sep 29, 2020
Closed
@vindl
Copy link
Member

vindl commented Sep 29, 2020

Now that you mention it, I find it strange that the navigation-panel is being rendered inside the skeleton__body; Wouldn't it make more sense to render it above the skeleton__header, as the first element within the the interface-skeleton? Taking this even further, perhaps the navigation-toggle__button doesn't belong in the skeleton__header either. I think moving these components outside the header and body could help with both the transition and an alignment issue that has popped up in #25630.

I filed a follow-up issue to track this in #25701.

@david-szabo97 david-szabo97 mentioned this pull request Sep 29, 2020
Merged
6 tasks
@jeyip jeyip mentioned this pull request Oct 6, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-szabo97 david-szabo97 david-szabo97 left review comments

@vindl vindl vindl approved these changes

@shaunandrews shaunandrews shaunandrews approved these changes

@mattwiebe mattwiebe Awaiting requested review from mattwiebe

@youknowriad youknowriad Awaiting requested review from youknowriad

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

Assignees

@Copons Copons

Labels
[Feature] Navigation Component A navigational waterfall component for hierarchy of items. Needs Design Feedback Needs general design feedback.
Projects
None yet
Milestone
Gutenberg 9.2
Development

Successfully merging this pull request may close these issues.
4 participants
@Copons @shaunandrews @vindl @david-szabo97