-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Patterns: remove "Manage all parts" page & link #60689
Conversation
Size Change: -2.26 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There will be a couple of pieces missing if we move forward with this... The "Manage all template parts" view supports the List layout, whereas other pattern views do not. We should decide whether it should be possible to view patterns in List layout and apply that across. Probably fine to do that separately if you prefer. In "Manage all template parts" there are dedicated views to filter by author. This filter is missing entirely in the pattern category views. Would it be possible to add it as a primary filter? |
So, there's a question here about how to do with classic themes that declare theme support for This is how they work in 6.5:
Gravacao.do.ecra.2024-04-12.as.16.11.45.movThis is how they work in this PR:
Gravacao.do.ecra.2024-04-12.as.16.10.00.movFor this PR, the current state could be enough. However, it raised a few questions for me:
cc @SaxonF @jameskoster @youknowriad |
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. |
Yeah, I mentioned this in the other PR. I'm happy to implement this as a follow-up if we want, but shouldn't be a blocker IMO.
mmm, how so? I've added it in a previous step at #60372 Gravacao.do.ecra.2024-04-12.as.16.35.31.mov |
Flaky tests detected in 2ea07e2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8750686569
|
My build must not have compiled or something; I wasn't seeing it before, but I do see it now :)
Since the idea is to consolidate the pattern/template part concepts I think ideally the "All patterns" view will include template parts down the road. Is that technically feasible?
That's a very good question. Considering my previous point, I think your suggestion makes sense. cc @t-hamano on this since you've worked on the classic theme UX. |
This is a planned feature and is being tracked in #52150. For future roadmap, please see comments here. In fact, even in the classic theme, access to the site editor's pattern page (
This should also be possible. However, as mentioned earlier, we should also consider scenarios where the Gutenberg plugin is used on lower WP versions. |
Thanks for the ping 👍 I think @t-hamano sums up the current situation well. We should be able to improve some of these things but as I understand it, we'll need to ensure Gutenberg's minimum supported version is 6.5 before the consolidation can completely happen. |
Agree with @jameskoster that end goal is to simplify and treat template parts as synced patterns that belong to special categories (e.g header). In the short term though, if we need to fill a functionality gap, could we add an "all" category under template parts? Separately, as part of this work, could we move template parts above patterns to give them a little more prominence? |
#60775 adds a "All template parts" section in the sidebar as a previous step so it's available and can be used in this PR for hybrid themes. I'll rebase this when 60775 lands. |
@t-hamano @aaronrobertshaw thanks so much for the context. This PR removes the existing "Manage all template parts" link/page, but it also redirects My understanding is that this PR makes progress towards using the site-editor patterns screen for hybrid themes: when that time comes, we'd just need to make everything (patterns&parts) visible. I'll rebase this PR when #60775 lands, everything will be a bit clearer then. |
In this regard, by adding a backward compatibility hook to the Gutenberg plugin, we might be able to immediately publish the new Patterns page to all classic themes without having to wait until Gutenberg supports core version 6.5. See this comment for details. I plan to tackle this challenge. |
I've also tested the backflows from the editor and this is how it works:
Gravacao.do.ecra.2024-04-22.as.12.12.29.mov
Gravacao.do.ecra.2024-04-22.as.13.19.54.mov
Gravacao.do.ecra.2024-04-22.as.12.13.12.mov
Gravacao.do.ecra.2024-04-22.as.13.20.34.mov |
Unless there's more use cases to support, I think this is working as expected. Is there anything else left or can this be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but you know this better than me. :)
title={ config[ postType ].title } | ||
description={ config[ postType ].description } | ||
backPath={ config[ postType ].backPath } | ||
title={ config.title } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: since there's no more branching based on postType, why not inline the values? title={ __( 'Manage templates' ) }
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed at 5192d86
const { setCanvasMode } = unlock( useDispatch( editSiteStore ) ); | ||
|
||
useInitEditedEntityFromURL(); | ||
|
||
const patternDetails = usePatternDetails( postType, postId ); | ||
const isTemplatePartsMode = useSelect( ( select ) => { | ||
return !! select( editSiteStore ).getSettings() | ||
.supportsTemplatePartsMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document supportsTemplatePartsMode
and/or state the intention of this const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2dbe1c5 should help.
actions={ | ||
<AddNewPattern | ||
canCreateParts={ isBlockBasedTheme } | ||
canCreatePatterns={ ! isTemplatePartsPath } | ||
/> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actions={ | |
<AddNewPattern | |
canCreateParts={ isBlockBasedTheme } | |
canCreatePatterns={ ! isTemplatePartsPath } | |
/> | |
} | |
actions={ | |
( isBlockBasedTheme || ! isTemplatePartsPath ) && ( | |
<AddNewPattern | |
canCreateParts={ isBlockBasedTheme } | |
canCreatePatterns={ ! isTemplatePartsPath } | |
/> | |
) | |
} |
How about changing to conditional rendering? Otherwise, the hidden field will be rendered even though there are no controls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done at 08e8ebe
title = templatePartArea?.label || __( 'All Template Parts' ); | ||
description = | ||
templatePartArea?.description || | ||
__( 'Includes every template part defined for any area.' ); | ||
title = templatePartArea?.label || __( 'Template Parts' ); | ||
description = templatePartArea?.description; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about making these text the same for hybrid themes in the template parts screen so before/after is the same. Though I now realize it's best to leave them as they are. Reverted the changes at 18c6a0b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left only two suggestions, but they all work as expected. I think there are some tasks to tackle after this is merged, do you have any plans?
I'm going to take a few days off this area: I need to prepare a talk I'll give by end of week + looking at improving the Do you have a sense of next steps for this? I presume we want to update the Patterns link for classic themes so they load the site-editor Patterns page. Then also remove |
Thanks for all the work here @oandregal. In terms of pattern / template part alignment I think there are a couple of outstanding details that it'd be good to address for 6.6 if possible:
|
I think so. I will be working on officially publishing the Site Editor Patterns page for all Classic themes in WP6.6. Detailed specifications are summarized in this comment. I believe it does not conflict with the improvements suggested by @jameskoster. |
@jameskoster I've added those tasks as part of the tracking issue, so they're not lost #59659 |
Part #59659 and #55083
What?
/wp_template_part/all
URL is still accessible to hybrid themes (classic themes that declare support forblock-template-part
)./patterns
URL is still accessible unofficially to classic themes (with or without support forblock-template-part
)./wp_template_part/all
URL, as before/patterns
URL, as before/patterns
URL, as beforeWhy?
How?
This PR updates the components that are used for that route. It now loads the code from Patterns (sidebar & content components). The reason for this is that in a future step hybrid themes will also have access to the Patterns from the site editor, so we'll load the same components.
Changes:
packages/edit-site/src/components/layout/router.js
PageTemplatesTemplateParts
toPagePatterns
and fromSidebarNavigationScreenTemplatesBrowse
toSidebarNavigationScreenPatterns
.packages/edit-site/src/components/sidebar-navigation-screen-patterns/index.js
. Configures the component depending on whether we're loading the/wp_template_part/all
URL (display only Template parts) or/patterns
(display everything, Patterns & Parts).packages/edit-site/src/components/page-patterns/index.js
. Configures the component depending on whether we're loading the/wp_template_part/all
URL (display only Template parts) or/patterns
(display everything, Patterns & Parts).packages/edit-site/src/components/sidebar-navigation-screen-pattern/index.js
. Updates the logic so that is goes back to/wp_template_part/all
if the theme is not a block theme.The "Manage template parts" screen used different code to render the Template parts than the "Patterns" screen uses. This PR removes all of that.
Changes:
packages/edit-site/src/components/sidebar-navigation-screen-templates-browse/
):index.js
: removes all the code related to parts.packages/edit-site/src/components/page-templates/
):packages/edit-site/src/components/page-templates-template-parts/
topackages/edit-site/src/components/page-templates/
.PageTemplatesTemplateParts
component toPageTemplates
.packages/edit-site/src/components/page-templates-template-parts/add-new-template-part.js
, as we already have the same code in the Patterns screen.index.js
: removes all the code that was used for template parts.Testing Instructions
Using a block theme:
Using a classic theme (for example, visit
localhost:8889
and select TwentyTwentyOne):http://localhost:8889/wp-admin/site-editor.php?path=%2Fpatterns
and verify that it works as described above. It's an unofficial link, but people expect it to work.Using a hybrid theme (for example, visit
localhost:8889
and select EmptyHybrid):Go to Appareance > Template parts and verify the site-editor Template Parts page is loaded and works as described above.
Visit the link
http://localhost:8889/wp-admin/site-editor.php?path=%2Fpatterns
and verify that it works as described above. It's an unofficial link, but people expect it to work.