Make current theme and editor settings available to route area resolvers #69299
+28
−7
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
t-hamano
added
[Type] Enhancement
|
Size Change: +89 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I haven't tested this, but can we resolve the wrapper issue by returning I don't have a strong opinion on Router package changes, but I'll vote for the simplest solution for this particular case. |
|
I have tried the code below: function HomePreview() {
return undefined;
}
export const homeRoute = {
name: 'home',
path: '/',
areas: {
sidebar: <SidebarNavigationScreenMain />,
preview: <HomePreview />,
mobile: <SidebarNavigationScreenMain />,
},
};I was hoping that the preview area wouldn't be rendered, but at this point
|
stokesman
reviewed
Feb 25, 2025
There was a problem hiding this comment.
I'm not 100% certain that this couldn’t some day be made unnecessary but it seems a sensible extensibility feature to add to the router and it’s a simple enough change. The router is also entirely private so I think we should go with it for now.
I have a few opinions on it though. I happened to explore this same concept recently when thinking about a solution for #69108.
|
Flaky tests detected in 9bcca28. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13544266093
|
|
Thanks for the feedback!
Although the route registration API is private and therefore they can remove this parameter in the future if they wish, I am also concerned about this. If we proceed more cautiously without making changes to the router package, I would like to suggest a consistent UI like the following:
I don't have a strong opinion on which approach to take, but I just want to avoid UI like this (e.g. unnatural resize handles) 😅
|
|
I am less concerned about empty screens or resize handles, than about the user accessing functionality that is broken for classic theme users. |
|
In time, isn't the plan that all of the Site Editor will work for all theme types? There will be only one theme type. So yes it should eventually be unnecessary, it will take time though. |
Mamaduka
reviewed
Feb 26, 2025
This is true, but the site editor will likely eventually be required to be made available to more than just administrators. In that case, we may want to control which screens are accessible based on user roles. This PR is also necessary to control access to stylebook based on theme support. Either way, I think a mechanism for controlling areas based on some conditions will be necessary in the future. |
|
If I remember correctly, the router is still private/internal API; if we find a better alternative to |
Yes.
|
Mamaduka
reviewed
Feb 27, 2025
Comment on lines
63
to
65
| matchResolverArgs={ { | ||
| siteData: { currentTheme }, | ||
| } } |
There was a problem hiding this comment.
Each time this component re-renders, the matchResolverArgs will be a new value, even though the currentTheme value never changes. This will invalidate memoization in useMatch, resulting in a new match.
I'm unsure what consequences this can have on the Site Editor app; it would be hard to tell without proper testing. This makes me hesitant to approve this; just days away from Beta 1.
We could try and memoize (useMemo) the matchResolverArgs value, just like beforeNavigate.
There was a problem hiding this comment.
I had noted this too—latter part of #69299 (comment).
The value could be memoized before passing it as a simple safeguard. However, right now, it doesn’t seem to be an issue. App doesn’t rerender any more often so matchResolverArgs remains practically stable. I could be overlooking something but testing with this diff is what I did to try proving it:
Log on change of matchResolverArgs
diff --git a/packages/router/src/router.tsx b/packages/router/src/router.tsx
index 8fbf6e609f..1750989f4f 100644
--- a/packages/router/src/router.tsx
+++ b/packages/router/src/router.tsx
@@ -12,6 +12,7 @@ import {
useContext,
useSyncExternalStore,
useMemo,
+ useInsertionEffect,
} from '@wordpress/element';
import {
addQueryArgs,
@@ -157,7 +158,14 @@ export default function useMatch(
matchResolverArgs: Record< string, any >
): Match {
const { query: rawQuery = {} } = location;
-
+ useInsertionEffect(
+ () =>
+ console.log(
+ 'using match: matchResolverArgs changed!',
+ matchResolverArgs
+ ),
+ [ matchResolverArgs ]
+ );
return useMemo( () => {
const { [ pathArg ]: path = '/', ...query } = rawQuery;
const result = matcher.recognize( path )?.[ 0 ];
There was a problem hiding this comment.
I think it’s safer to memoize now. It can be missed later and can have unexpected side effects.
There was a problem hiding this comment.
Thanks for the research everyone! I'll try out your suggestions tomorrow.
Should we freeze this PR and try the alternative approach suggested in this comment? In other words, we try a similar approach to #69005:
If navigation within the site editor is properly controlled, users should never see this screen unless they access it directly via a URL. I think this approach can be worked on even after the beta release, because it's a "bug fix" that doesn't add any new APIs. What do you think? |
stokesman
approved these changes
Feb 28, 2025
t-hamano
force-pushed
the
router/add-context
branch
from
080f230 to
6a1bfeb
Compare
t-hamano
force-pushed
the
router/add-context
branch
from
6a1bfeb to
00766d0
Compare
|
Or, you could let enable the theme support if theme.json exists, and still:
|
t-hamano
changed the title
This was referenced Mar 2, 2025
Ideally, we would add a new field to the REST API endpoint. This is about here. if ( rest_is_field_included( 'has_theme_json', $fields ) ) {
$data['has_theme_json'] = wp_theme_has_theme_json();
}However, I am hesitant to add such a new field just before the Beta release. |
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.
Preparation to move #69005 and #69043 forward
What?
This PR adds a new
contextmatchResolverArgsparameter to theuseMatchhook.This will allow us to control whether or not to render a particular area based on the theme, for example.
Why?
Currently, the site editor determines what to render in which area based on the path, and if the area is not
undefined, it renders the component in some wrapper element (example).As attempted in #69005 (review), there are cases where we don't want to render a specific area itself based on a condition. For example, whether it's a block theme.
However, even if a component returns null based on a certain condition, like this, the result is not equal to
undefined, so the area wrapper itself will be rendered:How?
contextmatchResolverArgsparameter to theuseMatchhook. This context can be used to pass any information through theRouterProvider.Testing Instructions
For example, make the following changes: You should now have access to the information provided by your router provider:
In #69005, we are trying to control which screens are exposed to the classic theme. For example, to not expose the navigation screen to the classic theme, we should be able to disable the specific area itself as follows:
In #69043, we are trying to control access to the StyleBook based on theme support. For example, we should be able to disable the specific area itself as follows: