Classic themes: prevent access to parts of the Site Editor #69005
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
This comment was marked as resolved.
This comment was marked as resolved.
|
Size Change: +383 B (+0.02%) 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. |
carolinan
added
[Type] Bug
|
Flaky tests detected in d497b8d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13494851693
|
|
I am not found of this solution. |
|
I could use some help with figuring out what needs to be tested with hybrid themes. |
I think that for screens that are accessible via URL but were previously prohibited from being accessed from classic/hybrid themes, only an error message should be displayed.
Does this mean which screens we should test? If so, this route list should help. |
|
I have not worked with hybrid themes for a long time, I am unsure of what features are intended to be available: |
|
I am working on hiding the preview of the single pages, since you can click on the preview and edit the page in the Site Editor and that should be prevented. |
|
To test hybrid themes, I added |
|
Still, I am not so found of this code repetition, there may be better ways to solve this. |
|
A lot of time spent on this issue was used to identify which file contained the code for which screen. I wonder if we could make this easier. What would be a suitable way to document this? |
t-hamano
reviewed
Feb 20, 2025
There was a problem hiding this comment.
I think that instead of adding conditional statements to the Editor component or PageTemplates component themselves, we should add similar processing to the routes itself. This should prevent unnecessary hooks from being executed.
Taking the Navigation route as an example, we could probably implement it like this:
Example
function NavigationContent() {
// In the classic theme, display error text
if ( ! isBlockBasedTheme ) {
return __( 'The theme you are currently using is not compatible with the Site Editor.' );
}
// In the block theme, we don't want to show any content
return null;
}
function NavigationPreview() {
// In the classic theme, we don't want to show any preview
if ( ! isBlockBasedTheme ) {
return null;
}
// In the block theme, render the resizable editor preview
return <Editor />;
}
export const navigationRoute = {
name: 'navigation',
path: '/navigation',
areas: {
content: <NavigationContent />,
preview: <NavigationPreview />,
},
};Unfortunately, this approach doesn't work: the areas.content or areas.preview wrappers will always be rendered, even if the component returns null:
We want to conditionally render areas.content and areas.preview depending on whether the theme is a block theme or not. For example, we may want to render only areas.content in the classic theme and only areas.preview in the block theme.
However, we cannot completely hide it because the wrapper element is always rendered just by defining the content or preview field.
If you know any good approaches, please let us know 🙏
packages/edit-site/src/components/sidebar-navigation-screen-templates-browse/index.js
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This limitation also removed the preview on the start screen.
I've been thinking about ways to solve this problem, and the idea I came up with is to add the following logic to the This should allow you to completely hide both the content and the preview when you access the Pages screen in the classic theme, for example. However, I'm not sure if this is the right approach 🤷 ExampleNote that I have simplified the code. function Layout() {
const { query, name: routeKey, areas, widths } = useLocation();
const isBlockBasedTheme = useSelect(
( select ) => select( coreStore ).getCurrentTheme()?.is_block_theme,
[]
);
const screensForClassicTheme = [ 'patterns', 'styles' ];
const shouldRenderContentArea =
areas.content &&
( isBlockBasedTheme || ! screensForClassicTheme.includes( routeKey ) );
const shouldRenderPreviewArea =
areas.preview &&
( isBlockBasedTheme || ! screensForClassicTheme.includes( routeKey ) );
return (
<>
<div className="edit-site-layout">
<div className="edit-site-layout__content">
{ ! isMobileViewport &&
shouldRenderContentArea &&
canvas !== 'edit' && (
<div className="edit-site-layout__area"></div>
) }
{ ! isMobileViewport && shouldRenderPreviewArea && (
<div className="edit-site-layout__canvas-container"></div>
) }
</div>
</div>
</>
);
}Additional ping: @stokesman I believe you've been working on improving the site editor screen lately 🙏 |
|
If it is hidden, there wont be a message showing that the user does not have access 🤔 |
Sorry for the poor explanation, I was thinking of moving the error notification to the sidebar:
|
|
It doesn't work because its not visible on the narrow screens. |
|
Can we display the same message by using |
|
Would it be ok to try a different approach in a different PR? I think I'll have it ready by tomorrow at the latest. |
|
Yes I think it would be easiest to test it on a clean branch based on trunk. |
|
I came up with the idea that a more ideal solution would be to extend the router package itself: #69299 |
|
Thanks for working on this issue. I've submitted #69473, which is based on the new approach. |
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.
What?
This PR removes some content from specific screens in the Site Editor when a classic theme is active.
Note: This is an edge case since it means that the user is manually navigating to a screen that there is no direct link to in the default WordPress UI.
Closes #68950
Alternative to #68959
Why?
There are Site Editor features that doesn't work with classic themes yet, such as creating and editing block templates
or previewing the navigation menu.
How?
The PR adds checks for the theme type, and then hides the sidebar and content if the theme is not a block theme.
Testing Instructions
Activate a block theme and navigate to different Site Editor pages to check that there are no regressions.
Activate a classic theme.
These screens also needs to be tested on narrow screen widths.
Navigate to:
wp-admin/site-editor.php?p=%2Ftemplatewp-admin/site-editor.php?p=%2Fnavigationwp-admin/site-editor.php?p=%2Fstyleswp-admin/site-editor.php?p=%2Fpagesinge page:
wp-admin/site-editor.php?p=%2Fpage&postId= YOUR ID HEREsingle navigation menu:
wp-admin/site-editor.php?p=%2Fwp_navigation%2FYOUR ID HEREThere should be a message that says: "The theme you are currently using is not compatible with the Site Editor."
Screenshots or screencast
Updated screenshot February 24: The navigation screen showing the notice instead of the sidebar content.
