Classic Theme: Expose new Patterns page and remove Template Parts submenu #61080
Conversation
t-hamano
added
[Type] Enhancement
|
Size Change: -235 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
| function gutenberg_change_patterns_link_and_remove_template_parts_submenu_item() { | ||
| if ( ! wp_is_block_theme() ) { | ||
| global $submenu; | ||
| foreach ( $submenu['themes.php'] as $key => $item ) { |
There was a problem hiding this comment.
The index of the Template Parts submenu varies depending on the core version. So we need to explore the submenu with foreach() (See this changeset).
There was a problem hiding this comment.
I noticed that this line produces PHP warnings.
[02-May-2024 08:18:23 UTC] PHP Warning: Trying to access array offset on value of type null in ~/gutenberg/wp-content/plugins/gutenberg/lib/compat/wordpress-6.6/compat.php on line 18
[02-May-2024 08:18:23 UTC] PHP Warning: foreach() argument must be of type array|object, null given in ~/gutenberg/wp-content/plugins/gutenberg/lib/compat/wordpress-6.6/compat.php on line 18
| if ( path === '/wp_template_part/all' && postId ) { | ||
| return { isReady: true, postType: 'wp_template_part', postId, context }; | ||
| } |
There was a problem hiding this comment.
This code was added when DataViews was introduced to the Template Parts List page, so I think it's safe to remove it now (See #57952).
| [ '/wp_template', '/wp_template_part/all', '/pages' ].includes( | ||
| path | ||
| ) || | ||
| [ '/wp_template', '/pages' ].includes( path ) || |
There was a problem hiding this comment.
I'm a little unsure about this change, but is it safe to remove it?
There was a problem hiding this comment.
I don't think it is.
By simply removing the wp_template_part/all path from this list, that page/route will be treated as if it is an "editor page" so it has commands like "Open styles" registered for it which will result in a broken black screen.
There was a problem hiding this comment.
Reverted by 5eb3d61
t-hamano
force-pushed
the
site-editor/new-patterns-page-for-classic-theme
branch
from
bc4f52f
to
058d832
Compare
|
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. |
There was a problem hiding this comment.
Thanks for putting this together @t-hamano, appreciate the hard work 👍
For the most part, it tests as advertised for me:
✅ Appearance > Patterns appears as before
✅ Appearance > Template parts does not appear
✅ ?path=wp_tempalte_part/all displays the Patterns page
✅ Navigating back from pattern and template part details pages returns to Patterns
✅ The Patterns command navigates to the Patterns page in the site editor
✅ Options menu and block toolbar Manage Patterns links go to the Patterns page
✅ WP6.4 worked as expected
While testing though I did run into a couple of issues:
- When selecting a Pattern or Template Part and viewing its details page, navigating back no longer preselects the previously selected category.
Is this something that regressed during recent refactors in this area or am I misremembering how it all used to work? There's not much point to passing through the categoryId to the details page otherwise 🤔
- If a user lands on the Patterns page via
?path/wp_template_part/alland opens the command palette they'll get commands they shouldn't, e.g. Open styles, which if selected result in a black page.
| * Change the Patterns submenu link and remove the Template Parts submenu for | ||
| * the Classic theme. This function should not be backported to core, and should be | ||
| * removed when the required WP core version for Gutenberg is >= 6.6.0. |
There was a problem hiding this comment.
Once we have the wordpress-develop PR available, it might be handy to add a link to it here in this comment.
| @@ -45,7 +44,7 @@ export function useAdminNavigationCommands() { | |||
| label: __( 'Patterns' ), | |||
| icon: symbol, | |||
| callback: ( { close } ) => { | |||
| if ( isTemplatesAccessible && isBlockBasedTheme ) { | |||
| if ( isTemplatesAccessible ) { | |||
There was a problem hiding this comment.
I appreciate this PR is only removing the isBlockBasedTheme check but do you think it is worth adding a comment here to explain why a Patterns command is relying on if templates are accessible?
There was a problem hiding this comment.
Makes sense. I just reused this comment:
gutenberg/packages/patterns/src/components/patterns-manage-button.js
Lines 38 to 40 in 08a8156
There was a problem hiding this comment.
I'm not 100% sure that comment captures the situation here.
The isTemplatesAccessible value is set via useIsTemplatesAccessible which checks read permissions for templates. Additionally, it should probably refer to this as a command rather than a link.
There was a problem hiding this comment.
I updated it with a0e4add, but I'm not sure if I'm communicating it well 😅
| [ '/wp_template', '/wp_template_part/all', '/pages' ].includes( | ||
| path | ||
| ) || | ||
| [ '/wp_template', '/pages' ].includes( path ) || |
There was a problem hiding this comment.
I don't think it is.
By simply removing the wp_template_part/all path from this list, that page/route will be treated as if it is an "editor page" so it has commands like "Open styles" registered for it which will result in a broken black screen.
|
@aaronrobertshaw Thanks for the review!
I recorded the steps to reproduce the problem and you are saying this, right? 5bf17510002d4871c34089d0347d32d3.mp4Another thing I noticed is that when I get to the pattern editor, I skip the details page.
I think this is not the intended behavior, so I would like to submit a separate issue. |
I believe #61174 will fix this problem. |
That's correct. Whatever category was selected in the sidebar nav screen should be maintained when the user views the pattern/template part, then navigates back to the Patterns page. |
This issue was resolved in #61174. |
|
I've tested using WordPress 6.5 and 6.4 the three kind of themes (block, classic, hybrid). It works as advertised. The only question I have in terms of behavior is whether we still need support the |
A possible scenario is that a user has bookmarked this page, or a developer has included a link to this page in their product. Personally, I think it's okay to delete it eventually. I checked with WP Directory and found that a few plugins have code that includes the string |
That's good thinking. However, while the URL still exists it no longer serves the same purpose and has different data/capabilities:
What if we redirected requests to |
| /> | ||
| ) | ||
| } | ||
| title={ __( 'Patterns' ) } |
There was a problem hiding this comment.
Just above this return statement, there's a reference to the /wp_template_part/all URL we need to update. Hybrid themes no longer have the "Appearance > Template Parts" link in the menu after this PR and we don't load different data depending on the URL either.
There was a problem hiding this comment.
Code-wise this also seems ready to me.
I've left a question about redirecting /wp_template_part/all to /patterns so we can remove every other reference to it. This would be my preference as to avoid intentionally breaking any functionality that depends on it (the get-is-list-page logic). Now that it's not used anywhere it's bound to happen.
Approving anyway as this works and that detail can be addressed in a follow-up. This PR touches a few different files so the longer it stays open, the more conflicts it'll have.
|
@aaronrobertshaw @oandregal Thanks for the review! I would like to merge this PR to avoid causing further conflicts over time. I will be happy to follow up on the points raised in this comment, as well as the redirect from Additionally, according to this comment I plan to submit a ticket to do the following in core:
|
|
Core ticket: https://core.trac.wordpress.org/ticket/61109 |
Part of #52150 (See this comment)
What?
This PR makes the following changes to explore the new Patterns page for the Classic theme in WP6.6.
wp-admin/site-editor.php?path=/patterns)wp-admin/site-editor.php?path=/wp_template_part/all) is accessed.Why?
The Current Patterns page includes the Template Parts List, and I think we're ready to explore the new Patterns page for the classic theme.
How?
I originally thought I'd split this PR into multiple parts and just change the Patterns submenu link at first. However, in the hybrid theme, two links (Appearance > Patterns, Appearance > Template Parts) indicating the Site Editor would be displayed, which would confuse users, so I combined them into one PR.
Testing Instructions
Access the test environment (
http://localhost:8889/), enable the Emptyhybrid or Twenty Twenty-One, and perform the following tests.Dashboard
Routing
http://localhost:8889/wp-admin/site-editor.php?path=/wp_template_part/all. - The Patterns page should open.Patterns Page Link
All of the following should link to the new Patterns page.
Backward Compatibility
Downgrade WordPress core version to 6.3 or 6.4, which is currently supported by Gutenberg. It should work exactly the same as it did when the core version was 6.5.
Next Step
My understanding is that this is the last task in Guenberg.
Next, I plan to submit the core ticket below.