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

Site editor: Mount both wp_template and wp_template_part EntityProviders to avoid remounting #25870

Merged
merged 2 commits into from Oct 8, 2020

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Oct 6, 2020

Fixes: #25743

Description

EntityProvider uses a different context provider component for each entity type. Which results in a remount since we are changing the component.

Unfortunately, EntityProvider is at the top of our tree, which results in a full Editor remount. Causing all the states to be lost. To avoid the remount, we introduce both EntityProvider in the tree, rather than one dynamic component.

How has this been tested?

  • yarn wp-env start
  • Enable FSE
  • Open Site Editor
  • Click site logo to toggle the navigation sidebar

Types of changes

Bug fix

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 6, 2020

Size Change: +22 B (0%)

Total Size: 1.18 MB

Filename Size Change
build/edit-site/index.js 20.6 kB +22 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 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-editor/index.js 129 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 135 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/blocks/index.js 47.5 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.42 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.7 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.29 kB 0 B
build/edit-post/style.css 6.27 kB 0 B
build/edit-site/style-rtl.css 3.71 kB 0 B
build/edit-site/style.css 3.72 kB 0 B
build/edit-widgets/index.js 21.3 kB 0 B
build/edit-widgets/style-rtl.css 3 kB 0 B
build/edit-widgets/style.css 3 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.44 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.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@david-szabo97 david-szabo97 self-assigned this Oct 6, 2020
@david-szabo97 david-szabo97 changed the title [WIP] Site editor: Mount wp_template and wp_template_part EntityProviders all the time Site editor: Mount wp_template and wp_template_part EntityProviders all the time Oct 6, 2020
@david-szabo97 david-szabo97 changed the title Site editor: Mount wp_template and wp_template_part EntityProviders all the time Site editor: Mount both wp_template and wp_template_part EntityProviders to avoid remounting Oct 6, 2020
@david-szabo97 david-szabo97 marked this pull request as ready for review October 6, 2020 17:01
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

This tests well for me, I think we can merge it 🚀

My one thought is that it seems weird to have to wrap so many entity providers like this:

- entity provider (root/site)
  - entity provider (wp_template)
    - entity provider (wp_template_part)
      - entity provider (global styles)

I wonder if there is a way we can abstract that further into one component - or maybe let entity provider handle multiple entities? Not sure. It feels like it could be cleaner.

But I don't think we need to figure that out right now :)

@david-szabo97
Copy link
Member Author

I wonder if there is a way we can abstract that further into one component - or maybe let entity provider handle multiple entities? Not sure. It feels like it could be cleaner.

Definitely! A multi-entity provider would be great. We will need to discuss this with the core team.

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 also agree that a multi-entity provider would be awesome!
I'm not familiar enough with EntityProvider and its implication to have a good idea on how to build it though.

@david-szabo97 david-szabo97 merged commit 16b3c51 into master Oct 8, 2020
@david-szabo97 david-szabo97 deleted the fix/navigation-panel-remount-animation branch October 8, 2020 07:48
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Component: Animation triggers when switching between different entity types
3 participants