Skip to content

Site Editor - add query args for current context.#27124

Merged
Addison-Stavlo merged 6 commits intomasterfrom
try/site-editor-routing
Dec 4, 2020
Merged

Site Editor - add query args for current context.#27124
Addison-Stavlo merged 6 commits intomasterfrom
try/site-editor-routing

Conversation

@Addison-Stavlo
Copy link
Copy Markdown
Contributor

@Addison-Stavlo Addison-Stavlo commented Nov 20, 2020

Description

Initial stab at #22238. This PR allows us to load the site editor to a specific context via query args postType and postId

Based on @vindl's comment here, this PR has been changed from the original implementation.

The idea is that postType can be one of: "page", "post", "wp_template", "wp_template_part". Then the ID would be whichever post we mean to display. This covers several scenarios:

  1. If we wish to link directly to a "page", we use postType=page&postId=2. This is enough information for the site editor to resolve the correct template to display. Same for "post"
  2. If we wish to link directly to a template with no content, we can use postType=wp_template&postId=2. This will load the specified template without any page context set. Same for "wp_template_part".

Limitations:

  1. Does not support other context types yet, such as "category" or anything related to queries.
  2. If we need to resolve templates or template parts by slugs if there is no post ID, this will not work.

I think this is simple enough for a round one without having to cover the edge cases by serializing the JSON state into the URL. What do you think?

How has this been tested?

  • Load the site editor as you would normally.
  • select another entity in the nav panel, verify query args are added to the url.
  • reload the browser, verify the editor loads to entity corresponding to the query args.

Verify this works for various contexts including: categories, pages, posts, templates, and template parts.
Verify creating a new template updates the url as well.

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

github-actions bot commented Nov 20, 2020

Size Change: +329 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-editor/index.js 128 kB -85 B (0%)
build/block-library/editor-rtl.css 9.07 kB +193 B (2%)
build/block-library/editor.css 9.07 kB +192 B (2%)
build/block-library/index.js 148 kB -352 B (0%)
build/components/index.js 172 kB +24 B (0%)
build/edit-site/index.js 24.4 kB +357 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 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/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/style-rtl.css 8.34 kB 0 B
build/block-library/style.css 8.34 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 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.95 kB 0 B
build/core-data/index.js 15.2 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.98 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.49 kB 0 B
build/edit-post/style.css 6.47 kB 0 B
build/edit-site/style-rtl.css 4.06 kB 0 B
build/edit-site/style.css 4.06 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.6 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.63 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 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 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.93 kB 0 B
build/list-reusable-blocks/index.js 3.1 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.31 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 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.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 2.84 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@vindl vindl requested review from gziolo and youknowriad November 20, 2020 00:50
@vindl vindl modified the milestone: Gutenberg 9.5 Nov 20, 2020
@Addison-Stavlo Addison-Stavlo added [Status] In Progress Tracking issues with work in progress and removed [Status] In Progress Tracking issues with work in progress labels Nov 20, 2020
@Addison-Stavlo Addison-Stavlo changed the title Site Editor - try routing & query args. Site Editor - add query args for current context. Nov 24, 2020
Copy link
Copy Markdown
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I believe this will be useful for detecting the context for Query block as well.

This seems to be in a good direction but I need to test it more. I noticed a couple of things for start:

  1. In FSE navigation(sidebar) the description displays html tags. In templates-> index description.
  2. Having content as object in url makes it unreadable. I don't have something to propose yet but will look in more depth in general.

Copy link
Copy Markdown
Member

@david-szabo97 david-szabo97 left a comment

Choose a reason for hiding this comment

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

Love this! Thank you for working on it! ✅ 🙇

@noahtallen noahtallen force-pushed the try/site-editor-routing branch from 6288aaf to 2296e01 Compare November 26, 2020 21:56
@noahtallen
Copy link
Copy Markdown
Member

In 8468ac7, I switched to use @wordpress/url instead of the build-in methods. I think with these changes all PR feedback should be addressed! It seems to be working pretty well.

@noahtallen
Copy link
Copy Markdown
Member

Might need to update some e2e tests, but rerunning the failing ones first to make sure.

@noahtallen
Copy link
Copy Markdown
Member

noahtallen commented Nov 27, 2020

I verified that the e2e failure started happening elsewhere before this PR (see here)

Copy link
Copy Markdown
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

I tested this and works well for now. I think we can merge and continue on follow ups, especially for handling taxonomies. The change to not having in URL all the context seems right to me and makes it readable as well. Will need a rebase for green CI though (thanks to @noahtallen ) :)

Thanks for working on this 👍 !


return (
<>
<PostRouter />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this naming is very helpful. as in my mind this is something like FSERouter or SiteEditorRouter... It can be addressed later though but wanted to get some thoughts from others..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't have any strong reasons for choosing PostRouter other than lack of creativity. EditorRouter or something along the flavors you mentioned may be more fitting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or would QueryController, or URLQueryController make more sense? Any thoughts?

Copy link
Copy Markdown
Contributor Author

@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.

Having content as object in url makes it unreadable. I don't have something to propose yet but will look in more depth in general.

Agreed, however I was thinking it would be preferable to have an unreadable value for one of these over a simplification that only supports a subset of editor states. However, I think we can still have the best of both worlds.

continue on follow ups, especially for handling taxonomies.

We should still be able to support taxonomies without that unreadable value by adding a few more readable query args and using them to rebuild the page object we set on state. Or are there any other ideas on how to handle them? 🤔 Either way, I also think that will make sense as a follow up.


Thanks for working on this one @noahtallen ! Its looking good 😁

@noahtallen noahtallen force-pushed the try/site-editor-routing branch from 1f0f6fa to ed2a1cd Compare December 2, 2020 07:39
@ntsekouras
Copy link
Copy Markdown
Contributor

Is something blocking this PR?

@Addison-Stavlo
Copy link
Copy Markdown
Contributor Author

Is something blocking this PR?

Nope, sorry! We have been distracted with some unrelated duties this week. 😢 Il rebase.

@Addison-Stavlo Addison-Stavlo force-pushed the try/site-editor-routing branch from ed2a1cd to b074dec Compare December 3, 2020 16:14
@Addison-Stavlo
Copy link
Copy Markdown
Contributor Author

Rebased and updated the name. Just need tests to pass and a 👍 on the new component name and this should be good to merge. (also feel free to change it if you think EditorRouter or SiteEditorRouter would still be more fitting).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants