Site Editor: Clear the active menu state when closing the sidebar #25957

Copons · 2020-10-08T14:44:52Z

Description

The Site Editor's navigation sidebar keeps its active menu state in the edit-site store.

Though, the state wasn't cleared when closing the sidebar, so reopening it would automatically enter whatever menu was left active before.

This PR clears the active menu state of the Site Editor sidebar when it's closed, so that reopening it will enter the root menu.

How has this been tested?

Enable the Site Editor experiment.
Open the Site Editor.
Toggle the sidebar (click on the top-left WP/site icon).
Navigate into a nested menu.
Close the sidebar (click on the same WP/site icon).
Reopen the sidebar.
It should start from the "root" menu, instead of the one left open before.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

github-actions · 2020-10-08T14:54:26Z

Size Change: +593 B (0%)

Total Size: 1.19 MB

Filename	Size	Change
`build/block-editor/index.js`	129 kB	+10 B (0%)
`build/block-editor/style-rtl.css`	10.9 kB	-36 B (0%)
`build/block-editor/style.css`	10.9 kB	-38 B (0%)
`build/block-library/index.js`	145 kB	+544 B (0%)
`build/blocks/index.js`	47.6 kB	+73 B (0%)
`build/edit-post/index.js`	306 kB	+5 B (0%)
`build/edit-post/style-rtl.css`	6.29 kB	+3 B (0%)
`build/edit-post/style.css`	6.28 kB	+5 B (0%)
`build/edit-site/index.js`	21 kB	+14 B (0%)
`build/edit-widgets/index.js`	21.2 kB	+13 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.54 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`	668 B	0 B
`build/block-directory/index.js`	8.55 kB	0 B
`build/block-directory/style-rtl.css`	943 B	0 B
`build/block-directory/style.css`	942 B	0 B
`build/block-library/editor-rtl.css`	8.65 kB	0 B
`build/block-library/editor.css`	8.65 kB	0 B
`build/block-library/style-rtl.css`	7.66 kB	0 B
`build/block-library/style.css`	7.65 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/components/index.js`	169 kB	0 B
`build/components/style-rtl.css`	15.5 kB	0 B
`build/components/style.css`	15.5 kB	0 B
`build/compose/index.js`	9.43 kB	0 B
`build/core-data/index.js`	12 kB	0 B
`build/data-controls/index.js`	685 B	0 B
`build/data/index.js`	8.6 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/index.js`	10.6 kB	0 B
`build/edit-navigation/style-rtl.css`	868 B	0 B
`build/edit-navigation/style.css`	871 B	0 B
`build/edit-site/style-rtl.css`	3.73 kB	0 B
`build/edit-site/style.css`	3.73 kB	0 B
`build/edit-widgets/style-rtl.css`	3.02 kB	0 B
`build/edit-widgets/style.css`	3.02 kB	0 B
`build/editor/editor-styles-rtl.css`	492 B	0 B
`build/editor/editor-styles.css`	493 B	0 B
`build/editor/index.js`	45.5 kB	0 B
`build/editor/style-rtl.css`	3.85 kB	0 B
`build/editor/style.css`	3.84 kB	0 B
`build/element/index.js`	4.45 kB	0 B
`build/escape-html/index.js`	734 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.54 kB	0 B
`build/is-shallow-equal/index.js`	710 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`	790 B	0 B
`build/redux-routine/index.js`	2.85 kB	0 B
`build/rich-text/index.js`	13 kB	0 B
`build/server-side-render/index.js`	2.6 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.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

david-szabo97 · 2020-10-08T16:08:05Z

🤔 Actually I prefer it to open the last active menu. Anyway, I'm OK with with clearing the state. Maybe it's better a11y wise?

jeyip

I tested template and template part navigation from the sidebar. Closing and reopening the sidebar always reset the state as expected.

✅ Chrome
✅ Firefox
✅ Edge
✅ Safari
✅ IE11

jeyip · 2020-10-08T18:56:13Z

Actually I prefer it to open the last active menu. Anyway, I'm OK with with clearing the state. Maybe it's better a11y wise?

I don't have a strong opinion, but I'm curious about this as well. Do you happen to know the reasoning @Copons?

jeyip · 2020-10-08T19:01:00Z

packages/edit-site/src/components/header/navigation-toggle/index.js

 	if ( ! isActive ) {
 		return null;
 	}

+	const handleClick = ( event ) => {
+		if ( isOpen ) {
+			setNavigationPanelActiveMenu();


Non-blocking:

Definitely nitpicky, but I would prefer invoking setNavigationPanelActiveMenu with "root" (like so setNavigationPanelActiveMenu("root")) because it is more explicit. Curious to hear your thoughts though.

My reasoning would be that root is part of the component's logic, and it shouldn't be a concern of the consumer.

On the other hand, the consumer still needs to know of that keyword to create nested menus, so yeah I guess it's not really an essential change.
I'll update to add root back into the navigation panel.

jeyip

Could we update the specs for packages/edit-site/src/store/test/reducer.js?

Edit: I accidentally approved this early, but +1 from me whenever the specs are set up 🙂

Copons · 2020-10-09T10:51:29Z

@david-szabo97 @jeyip
It's partly a personal preference, but it should also be an usability improvement.

The active menu is not necessarily connected to the current editor state (e.g. I can select a template, then navigate into another menu, without changing the active template).
If anything, when I reopen the sidebar, I'd rather see the menu containing the active template, than a possibly random one.
But...
The root menu contains the "Back to Dashboard" button, which is arguably the most essential one of the entire sidebar.
If the user just wants to exit the editor and the sidebar opens in a nested menu, the user would need to navigate up to root to eventually find the "Back" button.
Though, there are no indications anywhere that its in the root menu, so we would have to hope that the user noticed the "Back" button before navigating around.
The hierarchy of the navigation also conveys a context.
Say the user navigated to the page templates menu.
Theme -> Templates -> Pages makes it clear enough that we are looking at page templates.
But reopening straight into Pages is possibly confusing: are those pages or page templates?
All the above are also (anecdotally) a11y concerns.
If navigating up to root using a mouse and viewing all buttons and labels is simple enough, figuring it out with a screen reader probably becomes extremely complicated.

david-szabo97 · 2020-10-09T13:23:47Z

That all makes sense. Especially this

If navigating up to root using a mouse and viewing all buttons and labels is simple enough, figuring it out with a screen reader probably becomes extremely complicated.

Once tests are passing I'll review again and you might get an approval from me 😛

jeyip · 2020-10-09T17:51:01Z

The root menu contains the "Back to Dashboard" button, which is arguably the most essential one of the entire sidebar.

reopening straight into Pages is possibly confusing: are those pages or page templates?

navigating up to root...with a screen reader probably becomes extremely complicated.

Thanks for elaborating Jacopo! That all makes sense to me as well. 🙂

ntsekouras

Looks good 👍

vindl

🚀

Copons · 2020-10-13T11:35:46Z

Thanks for the merge @ntsekouras! 🙇‍♂️

Clear the active menu state on sidebar close

1976cfc

Copons added the [Feature] Full Site Editing label Oct 8, 2020

Copons requested review from vindl, david-szabo97 and jeyip October 8, 2020 14:44

Copons requested a review from youknowriad as a code owner October 8, 2020 14:44

Copons self-assigned this Oct 8, 2020

jeyip reviewed Oct 8, 2020

View reviewed changes

jeyip approved these changes Oct 8, 2020

View reviewed changes

Explicitly use root menu in the Nav component consumer

3afd290

Mock useDispatch to prevent failing tests

886fbbd

ntsekouras approved these changes Oct 12, 2020

View reviewed changes

david-szabo97 approved these changes Oct 12, 2020

View reviewed changes

vindl approved these changes Oct 12, 2020

View reviewed changes

ntsekouras merged commit 91ab6fc into master Oct 13, 2020

ntsekouras deleted the update/site-editor-sidebar-state-on-close branch October 13, 2020 05:21

github-actions bot added this to the Gutenberg 9.2 milestone Oct 13, 2020

david-szabo97 mentioned this pull request Oct 13, 2020

Move left sidebar state to redux #26003

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Editor: Clear the active menu state when closing the sidebar #25957

Site Editor: Clear the active menu state when closing the sidebar #25957

Copons commented Oct 8, 2020 •

edited

Loading

github-actions bot commented Oct 8, 2020 •

edited

Loading

david-szabo97 commented Oct 8, 2020

jeyip left a comment

jeyip commented Oct 8, 2020

jeyip Oct 8, 2020 •

edited

Loading

Copons Oct 9, 2020

jeyip left a comment •

edited

Loading

Copons commented Oct 9, 2020

david-szabo97 commented Oct 9, 2020

jeyip commented Oct 9, 2020 •

edited

Loading

ntsekouras left a comment

vindl left a comment

Copons commented Oct 13, 2020

Site Editor: Clear the active menu state when closing the sidebar #25957

Site Editor: Clear the active menu state when closing the sidebar #25957

Conversation

Copons commented Oct 8, 2020 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

github-actions bot commented Oct 8, 2020 • edited Loading

david-szabo97 commented Oct 8, 2020

jeyip left a comment

Choose a reason for hiding this comment

jeyip commented Oct 8, 2020

jeyip Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Copons Oct 9, 2020

Choose a reason for hiding this comment

jeyip left a comment • edited Loading

Choose a reason for hiding this comment

Copons commented Oct 9, 2020

david-szabo97 commented Oct 9, 2020

jeyip commented Oct 9, 2020 • edited Loading

ntsekouras left a comment

Choose a reason for hiding this comment

vindl left a comment

Choose a reason for hiding this comment

Copons commented Oct 13, 2020

Copons commented Oct 8, 2020 •

edited

Loading

github-actions bot commented Oct 8, 2020 •

edited

Loading

jeyip Oct 8, 2020 •

edited

Loading

jeyip left a comment •

edited

Loading

jeyip commented Oct 9, 2020 •

edited

Loading