Site editor - try redirecting to homepage before the react render #37248
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
changed the title
talldan
commented
Dec 9, 2021
| const { postType } = params; | ||
| return ( | ||
| ! getIsListPage( params ) && | ||
| ! [ 'post', 'page', 'wp_template', 'wp_template_part' ].includes( |
There was a problem hiding this comment.
I don't really get why the site editor is limited to these post types 🤔
(This replicates the else case of the effect where showHomepage was previously called in URLQueryController)
|
Size Change: -58 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Dec 9, 2021
|
|
||
| export default function HomepageRedirect() { | ||
| const params = useSelect( | ||
| ( select ) => select( editSiteStore ).getHomepageParams(), |
There was a problem hiding this comment.
This is an awesome improvement!
I'm wondering though, if we could select it upfront before rendering the React app like you mentioned. We can use resolveSelect and await for the promise to resolve before rendering the app. As long as the request is properly preloaded, I think the promise should resolve in just a few ticks. And then we won't need the loading state either. We still need this selector in React though for client side navigations.
There was a problem hiding this comment.
Yeah, I figured this should be possible, but I needed a bit more time. I managed to get it working with the latest commit though.
I went with using apiFetch for most of the api calls, as a resolver/selector combo feels unnecessarily complicated.
What do you think? It could probably use a bit of refining and error handling at the moment, and the preloading is still not working for wp/v2/settings for some reason.
There was a problem hiding this comment.
LGTM 👍 ! This feels like the right approach, at least until #36873 😜 .
| export default function EditSiteApp( { reboot } ) { | ||
| return ( | ||
| <SlotFillProvider> | ||
| <UnsavedChangesWarning /> | ||
|
|
||
| <Routes> | ||
| { ( { params } ) => { | ||
| if ( getNeedsHomepageRedirect( params ) ) { | ||
| return <HomepageRedirect />; |
There was a problem hiding this comment.
Maybe we should only render it in /editor/index.js? Since that we won't need to redirect if we're already on the list page.
There was a problem hiding this comment.
True, though rendering it here makes it easier to bypass all the react hooks in the Editor component.
There was a problem hiding this comment.
This is no longer an issue with the latest commit, as there's no component now 😄
talldan
changed the title
talldan
added
[Feature] Site Editor
talldan
commented
Dec 13, 2021
Comment on lines
+29
to
+40
| const { | ||
| show_on_front: showOnFront, | ||
| page_on_front: frontpageId, | ||
| } = siteSettings; | ||
|
|
||
| // If the user has set a page as the homepage, use those details. | ||
| if ( showOnFront === 'page' ) { | ||
| return { | ||
| postType: 'page', | ||
| postId: frontpageId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Most of this code is replicating showHomepage, but I wasn't sure how to test this as there's no customizer where this setting can be changed. Is it still possible to configure a homepage in this way?
There was a problem hiding this comment.
I think you can change some of these via Settings -> Reading -> Your homepage displays.
|
@ockham, do you think we can use |
talldan
force-pushed
the
try/changing-show-homepage-routing
branch
from
76d8810
to
7cdd4be
Compare
talldan
added
[Type] Code Quality
|
I've rebased this. What do you think @kevin940726 and @Mamaduka, is it worth getting this in? |
Apologies, I've seen your ping only now. |
More like to load the current page context in Site Editor via editor settings. I did a little more testing today, and it should work, but any help is appreciated 🙇 |
|
This seems to work-ish (patch against diff --git a/src/wp-admin/menu.php b/src/wp-admin/menu.php
index 5d550fb55d..b31409b434 100644
--- a/src/wp-admin/menu.php
+++ b/src/wp-admin/menu.php
@@ -203,6 +203,8 @@ if ( ! is_multisite() && current_user_can( 'update_themes' ) ) {
$submenu['themes.php'][5] = array( sprintf( __( 'Themes %s' ), $count ), $appearance_cap, 'themes.php' );
if ( wp_is_block_theme() ) {
+ $home_template = resolve_block_template( 'home', array(), '' );
+
$submenu['themes.php'][6] = array(
sprintf(
/* translators: %s: "beta" label */
@@ -210,7 +212,13 @@ if ( wp_is_block_theme() ) {
'<span class="awaiting-mod">' . __( 'beta' ) . '</span>'
),
'edit_theme_options',
- 'site-editor.php',
+ add_query_arg(
+ array(
+ 'postType' => 'wp_template',
+ 'postId' => $home_template->id
+ ),
+ 'site-editor.php'
+ ),
);
}
Needs a bit more work though since it doesn't quite reflect the template hierarchy yet. (Arguably, the extra logic should be absorbed into |
|
Thanks @Mamaduka! |
8 tasks
talldan
added
the
Backport to WP Minor Release
|
Cherry-picked for 5.9.1. |
Mamaduka
removed
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Feb 16, 2022
…7248) * Replace showHomepage with a selector and dedicate redirect component * Remove show homepage * Remove component based redirect, instead use an async function prior to rendering app * Use a default export * Pass siteUrl in as a param
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related #36873
After #36488, the way routing works in the site editor has changed significantly.
The store state is now mostly derived from the query parameters in the URL. Previously it was the other way around where a change to the store state caused the
URLQueryControllercomponent to update the URL.The
showHomepageaction is an exception though. This is the action that's triggered when the site editor is visited with nopostTypeandpostIdquery params.This PR removes that action in favour of a handling the updating the location to the homepage when the application is initialized. The idea being that it's good for the application to have the right state before anything else happens. It should mean there are fewer chances of bugs like the redirect loop one that briefly happened in #36488, since everything the app does is derived from having the right
postTypeandpostIdparams at the start.A caveat is that I'm not all that familiar with this code, so it needs some critique! There might be some edge cases I'm missing.
How has this been tested?
Types of changes
Code quality
Checklist:
*.native.jsfiles for terms that need renaming or removal).