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

Edit Site: Refactor business logic into store. #22844

Merged
merged 3 commits into from
Jun 3, 2020

Conversation

epiqueras
Copy link
Contributor

Description

This PR refactors all of the edit-site business logic and editor state into the store.

It also moves a local utility, getPathAndQueryString, into the @wordpress/url package.

How has this been tested?

Tests were written for all the new functions, and it was verified that nothing changed in the site editor.

Types of Changes

New Feature: @wordpress/url now has a getPathAndQueryString utility.

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 Jun 2, 2020

Size Change: +1.09 kB (0%)

Total Size: 1.12 MB

Filename Size Change
build/annotations/index.js 3.62 kB +1 B
build/api-fetch/index.js 3.4 kB +1 B
build/autop/index.js 2.83 kB -1 B
build/block-directory/index.js 6.48 kB +4 B (0%)
build/block-editor/index.js 106 kB +40 B (0%)
build/block-library/index.js 126 kB +27 B (0%)
build/blocks/index.js 48.2 kB +5 B (0%)
build/components/index.js 190 kB +7 B (0%)
build/compose/index.js 9.31 kB -1 B
build/core-data/index.js 11.4 kB +8 B (0%)
build/data-controls/index.js 1.29 kB +2 B (0%)
build/data/index.js 8.44 kB +10 B (0%)
build/edit-navigation/index.js 8.19 kB +5 B (0%)
build/edit-post/index.js 302 kB +5 B (0%)
build/edit-site/index.js 15 kB +899 B (5%) 🔍
build/edit-widgets/index.js 8.88 kB +2 B (0%)
build/editor/index.js 44.7 kB +27 B (0%)
build/element/index.js 4.65 kB +4 B (0%)
build/format-library/index.js 7.71 kB +1 B
build/keyboard-shortcuts/index.js 2.51 kB -1 B
build/keycodes/index.js 1.94 kB +1 B
build/media-utils/index.js 5.29 kB -4 B (0%)
build/notices/index.js 1.79 kB +1 B
build/nux/index.js 3.4 kB -2 B (0%)
build/plugins/index.js 2.56 kB +1 B
build/rich-text/index.js 14.8 kB +1 B
build/server-side-render/index.js 2.68 kB +3 B (0%)
build/url/index.js 4.06 kB +39 B (0%)
build/viewport/index.js 1.84 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 787 B 0 B
build/block-directory/style.css 787 B 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 7.87 kB 0 B
build/block-library/editor.css 7.88 kB 0 B
build/block-library/style-rtl.css 7.69 kB 0 B
build/block-library/style.css 7.68 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 19.5 kB 0 B
build/components/style.css 19.5 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/style-rtl.css 5.43 kB 0 B
build/edit-post/style.css 5.43 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

This is looking great and cleans things up quite a bit. Thank you!

Reading through the code changes things look good.

From what I can tell mostly everything in the site editor works as expected in testing. However, I notice that when I test the template switcher's "Overwrite Template" / "Revert to Parent" functionality for a page, the revert doesn't go through as on Master. Eventually get a 410 console error with Uncaught (in promise) {code: "rest_already_trashed", message: "The post has already been deleted.", data: {…}}

@epiqueras
Copy link
Contributor Author

Oops, I missed that part while refactoring. All done now 😄

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the quick fix there too!

Ive gone through and tested everything I can think of again in the site editor, and it's all working as expected. After combing through the code again, things look good to me!

Comment on lines +126 to +135
/**
* Returns the current template IDs.
*
* @param {Object} state Global application state.
*
* @return {number[]} Template IDs.
*/
export function getTemplateIds( state ) {
return state.templateIds;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS, the getTemplateIds selector isn't used anywhere. We could consider dropping it (and the corresponding templateIds reducer, plus potentially some related actions, resolvers, and editorInitialState).

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Jun 3, 2020

Choose a reason for hiding this comment

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

I think they might come in handy down the road though? If I'm understanding it right their values are being initialized from the edit-site-page.php, so now getting them will be easily accessible in future development. I'm assuming there are plans to use this (I'm kind of surprised its not already)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ockham Worked on removing it from edit-site state and instead use the getEntityRecords API with a new query parameter.

I think that, in #21878, we should remove both the templateIDs and templatePartIds selectors and reducers, as well as the code in edit-site-page that loads it.

@epiqueras epiqueras merged commit 655475a into master Jun 3, 2020
@epiqueras epiqueras deleted the update/move-edit-site-logic-into-store branch June 3, 2020 20:55
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone Jun 3, 2020
@youknowriad
Copy link
Contributor

Nice to see this PR. Thanks @epiqueras

@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
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