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

Remove more initial PHP state #23775

Merged
merged 24 commits into from
Jul 20, 2020
Merged

Conversation

noahtallen
Copy link
Member

Description

This PR looks at removing more initial PHP state for the site editor. Since things can be loaded more async now, I shored up many selectors and code paths to make sure they can handle null/undefined values.

This may also set us up more for loading different dynamic pages on the fly.

I will add comments where some of my concerns are.

How has this been tested?

Locally in edit site.

Screenshots

Types of changes

Code quality

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

Size Change: +137 B (0%)

Total Size: 1.15 MB

Filename Size Change
build/edit-site/index.min.js 16.9 kB +137 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.min.js 1.14 kB 0 B
build/annotations/index.min.js 3.67 kB 0 B
build/api-fetch/index.min.js 3.43 kB 0 B
build/autop/index.min.js 2.82 kB 0 B
build/blob/index.min.js 620 B 0 B
build/block-directory/index.min.js 7.63 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.min.js 124 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.58 kB 0 B
build/block-library/editor.css 7.57 kB 0 B
build/block-library/index.min.js 132 kB 0 B
build/block-library/style-rtl.css 7.77 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.min.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.min.js 3.1 kB 0 B
build/blocks/index.min.js 48.3 kB 0 B
build/components/index.min.js 200 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.6 kB 0 B
build/compose/index.min.js 9.67 kB 0 B
build/core-data/index.min.js 11.5 kB 0 B
build/data-controls/index.min.js 1.29 kB 0 B
build/data/index.min.js 8.45 kB 0 B
build/date/index.min.js 5.38 kB 0 B
build/deprecated/index.min.js 772 B 0 B
build/dom-ready/index.min.js 568 B 0 B
build/dom/index.min.js 3.23 kB 0 B
build/edit-navigation/index.min.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.min.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.min.js 9.35 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.min.js 45.1 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.min.js 4.65 kB 0 B
build/escape-html/index.min.js 733 B 0 B
build/format-library/index.min.js 7.72 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.min.js 2.13 kB 0 B
build/html-entities/index.min.js 621 B 0 B
build/i18n/index.min.js 3.56 kB 0 B
build/is-shallow-equal/index.min.js 711 B 0 B
build/keyboard-shortcuts/index.min.js 2.51 kB 0 B
build/keycodes/index.min.js 1.94 kB 0 B
build/list-reusable-blocks/index.min.js 3.12 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.min.js 5.32 kB 0 B
build/notices/index.min.js 1.79 kB 0 B
build/nux/index.min.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.min.js 2.56 kB 0 B
build/primitives/index.min.js 1.4 kB 0 B
build/priority-queue/index.min.js 789 B 0 B
build/redux-routine/index.min.js 2.85 kB 0 B
build/rich-text/index.min.js 13.9 kB 0 B
build/server-side-render/index.min.js 2.71 kB 0 B
build/shortcode/index.min.js 1.7 kB 0 B
build/token-list/index.min.js 1.27 kB 0 B
build/url/index.min.js 4.06 kB 0 B
build/viewport/index.min.js 1.85 kB 0 B
build/warning/index.min.js 1.14 kB 0 B
build/wordcount/index.min.js 1.17 kB 0 B

compressed-size-action

lib/edit-site-page.php Outdated Show resolved Hide resolved
packages/edit-site/src/store/actions.js Outdated Show resolved Hide resolved
packages/edit-site/src/store/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Left a few notes. I think that we might be able to drop a few more of the ancillary functions that have been introduced here as replacements for the previous initial state and tackle the problem at a higher level (hopefully):

I think the mechanism that we have in place to get the template for a given page should be relatively stable; we might not even need the extra logic involving show_on_front/page_on_front, but maybe we can basically just pass in the / route and rely on what is already there for template resolution (basically findTemplate and the related backend 'endpoint' doing all the heavy lifting).

@noahtallen
Copy link
Member Author

we might not even need the extra logic involving show_on_front/page_on_front

@ockham we do need to know what the post_id is (page_on_front) :) Though I'm not sure, does findTemplate give us that?

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.

I see that we have unit tests edit-site selectors, reducer, and actions. Should we update those in this PR?

@noahtallen
Copy link
Member Author

yeah, will do! (wanted to make sure the code was in a good place before modifying them)

@noahtallen noahtallen force-pushed the try/remove-site-editor-php-initial-state branch from c3d21df to f1b2f06 Compare July 9, 2020 18:49
@noahtallen
Copy link
Member Author

@ockham, Let me know what you think of the new approach. I added a showHomepage action which computes and sets the homepage. I think this is better than a general "setup state" action since it's more modular. (We now load show on front/page on front over the API)

@ockham
Copy link
Contributor

ockham commented Jul 13, 2020

A few unit tests are failing and need updating I think 😅

@noahtallen noahtallen force-pushed the try/remove-site-editor-php-initial-state branch 2 times, most recently from fa51d1a to 821ced6 Compare July 15, 2020 03:54
@ockham ockham force-pushed the try/remove-site-editor-php-initial-state branch from 8da55dc to 0d02191 Compare July 16, 2020 20:14
@noahtallen
Copy link
Member Author

noahtallen commented Jul 16, 2020

Nice, tests are looking good. (Wish I could dismiss stale reviews!)

@noahtallen noahtallen requested a review from ockham July 16, 2020 23:24
@noahtallen noahtallen merged commit 0169fe4 into master Jul 20, 2020
@noahtallen noahtallen deleted the try/remove-site-editor-php-initial-state branch July 20, 2020 19:34
@github-actions github-actions bot added this to the Gutenberg 8.7 milestone Jul 20, 2020
)
);
}
add_action( 'rest_api_init', 'register_site_editor_homepage_settings', 10 );
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is adding data to some REST API endpoint. So pinging @TimothyBJacobs for awareness.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ping! Doing this on rest_api_init should work, but it might be better to do it on init so it is always a registered setting. We can also safely drop the name parameter for show_in_rest. It'll default to the option name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, I will definitely make those changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants