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

Move left sidebar state to redux #26003

Merged
merged 2 commits into from
Oct 14, 2020
Merged

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Oct 10, 2020

Description

Instead of prop drilling the sidebar state, this tries moving it to redux, and it also tries separating "navigation" from "inserter" state. cc @Copons (I think you mentioned this here.)

How has this been tested?

  1. test that the changes in Site Editor: Clear the active menu state when closing the sidebar #25957 still work (i.e. the menu should go back to root when closing the panel)
  2. You can test that this is working by clicking the "view in navigation" button which is displayed in the template dropdown:

2020-10-13 17 55 05

Screenshots

Types of changes

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
Copy link

github-actions bot commented Oct 10, 2020

Size Change: +186 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/edit-site/index.js 21.1 kB +186 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.6 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 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/index.js 143 kB 0 B
build/block-library/style-rtl.css 7.71 kB 0 B
build/block-library/style.css 7.71 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/blocks/index.js 47.6 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.4 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.63 kB 0 B
build/core-data/index.js 12.1 kB 0 B
build/data-controls/index.js 684 B 0 B
build/data/index.js 8.63 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 569 B 0 B
build/dom/index.js 4.43 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-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.3 kB 0 B
build/edit-post/style.css 6.29 kB 0 B
build/edit-site/style-rtl.css 3.77 kB 0 B
build/edit-site/style.css 3.77 kB 0 B
build/edit-widgets/index.js 21.3 kB 0 B
build/edit-widgets/style-rtl.css 2.97 kB 0 B
build/edit-widgets/style.css 2.97 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 42.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 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 622 B 0 B
build/i18n/index.js 3.54 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/reusable-blocks/index.js 3.04 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.07 kB 0 B
build/viewport/index.js 1.75 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

This all makes sense to me. We could also use selectors to handle the "interdependent" behavior of the inserter and the navigation sidebar if we'd like. What do you think @Copons

@david-szabo97
Copy link
Member

david-szabo97 commented Oct 13, 2020

We already have a PR merged which saves the active menu into the store: #25906 and we had a change by Jacopo here: #25957

Changes look good and I see all the benefits of moving more state into the store. Could you do a rebase and make sure we only one have solution living the codebase? 😄 (Moving forward with your solution)

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

The requested changes should be fairly straightforward, but overall this works well and I'm totally into this solution!
I've even proposed something like this in a comment in the other PR: #25906 (comment) 🙂

As @david-szabo97 said: please rebase and replace the old state introduced in #25906 with this.
Also, please keep the logic introduced in #25957: when closing the navigation, we should revert the active menu to root (see reasons).

We could also use selectors to handle the "interdependent" behavior of the inserter and the navigation sidebar if we'd like.

@jeyip Could you please clarify this?
I think I'm misunderstanding what you're saying, but the reducer already makes inserter and navigation interdependent, by automatically closing one when we open the other. 🤔

packages/edit-site/src/store/actions.js Show resolved Hide resolved
@noahtallen
Copy link
Member Author

As @david-szabo97 said: please rebase and replace the old state introduced in #25906 with this.

I believe I had already incorporated SET_NAVIGATION_PANEL_ACTIVE_MENU into this PR, so I want to make sure I'm not missing something important or misunderstanding! I basically kept the existing action and selector, but added action.menu to the new navigationPanel store, wired in that action type, and also change the selector to point to the new state slice. So I think that aspect of things should be working.

Also, please keep the logic introduced in #25957: when closing the navigation, we should revert the active menu to root

Additionally, I just moved the logic which sets the menu back to "root" when closing it from the navigation toggle component into the reducer. It's nice how much the reducer simplifies things!

@noahtallen noahtallen marked this pull request as ready for review October 13, 2020 23:30
@jeyip
Copy link
Contributor

jeyip commented Oct 14, 2020

We could also use selectors to handle the "interdependent" behavior of the inserter and the navigation sidebar if we'd like.

@jeyip Could you please clarify this?
I think I'm misunderstanding what you're saying, but the reducer already makes inserter and navigation interdependent, by
automatically closing one when we open the other. 🤔

My mistake! You understood me perfectly @Copons.

  1. The reducers do already handle automatically closing one when we open the other.
  2. I was talking about handling the business logic in selectors, which, in retrospect, is totally unnecessary (and somewhat confusing).

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

I've pushed a minor update that replaces hardcoded menu names with their corresponding constants.
This works well for me so I'm approving the PR, but please double check if I've accidentally caused regressions. 🙂

@noahtallen
Copy link
Member Author

@Copons thanks for that! Love the change. I'll do another round of testing and then merge.

@noahtallen noahtallen force-pushed the try/moving-sidebar-state-to-redux branch 2 times, most recently from 14f0121 to 9d66759 Compare October 14, 2020 22:09
@noahtallen noahtallen force-pushed the try/moving-sidebar-state-to-redux branch 2 times, most recently from dbbdf2b to 66a93a2 Compare October 14, 2020 22:27
@noahtallen noahtallen merged commit 0d0f106 into master Oct 14, 2020
@noahtallen noahtallen deleted the try/moving-sidebar-state-to-redux branch October 14, 2020 22:54
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 14, 2020
@noahtallen noahtallen self-assigned this Oct 15, 2020
@noahtallen noahtallen added [Feature] Full Site Editing [Type] Code Quality Issues or PRs that relate to code quality labels Oct 15, 2020
@noahtallen noahtallen changed the title Try moving left sidebar state to redux Move left sidebar state to redux Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants