Move default template types and template part areas to REST API #66459
+559
−411
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
gigitux
force-pushed
the
fix/rest-template-parts
branch
from
5a5a7e9
to
86b2e8f
Compare
|
Size Change: +386 B (+0.02%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
gigitux
force-pushed
the
fix/rest-template-parts
branch
from
86b2e8f
to
95d6d1c
Compare
gigitux
force-pushed
the
fix/rest-template-parts
branch
from
7b905f6
to
5b83ef7
Compare
This was referenced Oct 25, 2024
gigitux
commented
Nov 1, 2024
Comment on lines
1718
to
1729
| export const __experimentalGetDefaultTemplatePartAreas = createRegistrySelector( | ||
| ( select ) => | ||
| createSelector( () => { | ||
| const areas = | ||
| select( coreStore ).getEntityRecord( 'root', '__unstableBase' ) | ||
| ?.defaultTemplatePartAreas || []; | ||
|
|
||
| return areas.map( ( item ) => { | ||
| return { ...item, icon: getTemplatePartIcon( item.icon ) }; | ||
| } ); | ||
| } ) | ||
| ); |
There was a problem hiding this comment.
I think that we should deprecate this selector and create a dedicated hook to be reused across the codebase: what do you think?
youknowriad
reviewed
Nov 4, 2024
packages/core-data/src/entities.js
Outdated
| @@ -31,6 +31,8 @@ export const rootEntitiesConfig = [ | |||
| 'site_icon_url', | |||
| 'site_logo', | |||
| 'timezone_string', | |||
| 'defaultTemplatePartAreas', | |||
| 'defaultTemplateTypes', | |||
There was a problem hiding this comment.
Looks like these are the only fields that are camelCase? should they follow the other pattern?
youknowriad
requested review from
Mamaduka,
jsnajdr,
jorgefilipecosta and
TimothyBJacobs
|
Do you think we can update this PR to target trunk directly (avoid all the changes related to the action)? That way we can land it first. |
Thanks! Sure! Thanks for tagging other folks! After that, I will make the changes, I will mark this PR ready to be reviewed! |
gigitux
force-pushed
the
fix/rest-template-parts
branch
from
344f380
to
6aaf160
Compare
|
Flaky tests detected in 2a04ba1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11683420405
|
gigitux
force-pushed
the
fix/rest-template-parts
branch
from
77c8204
to
69c3d6d
Compare
gigitux
added
[Type] Experimental
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
gigitux
added
[Package] Editor
gigitux
commented
Nov 5, 2024
| } ); | ||
| } ); | ||
|
|
||
| describe( '__experimentalGetTemplateInfo', () => { |
There was a problem hiding this comment.
I created the same unit tests for the getTemplateInfo function: 2a04ba1
|
Marking this PR as ready for review. Looking forward to your feedback! |
gigitux
requested review from
spacedmonkey,
ajitbohra and
nerrad
as code owners
|
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. |
gigitux
commented
Nov 5, 2024
| { | ||
| since: '6.7', | ||
| alternative: | ||
| "select('core/core-data').getEntityRecord( 'root', '__unstableBase' )?.default_template_part_areas", |
There was a problem hiding this comment.
Technically, missing the last piece of the logic:
areas.map( ( item ) => {
return { ...item, icon: getTemplatePartIcon( item.icon ) };
} );Not sure if I should add this line under the alternative key or simply remove it altogether.
|
I don't think the It also needs to be clarified to me why this change is needed. Maybe you can expand on that in the PR description or link an issue that has been discussed. Have we considered the |
Thanks for the comment! I updated the PR description explaining why this PR is necessary. Happy to provide more context if needed.
Not really. If you think that it makes sense, I could explore this path! |
|
@Mamaduka I suggested this approach, see here #65390 (comment) What would be the right endpoint for you for these general filtrable non editable settings ? I can't think of better than the index personally. |
|
For me the templates endpoint is specific to a post type, so it doesn't really make sense unless we make two API calls, one for template parts and one for templates. That would work for me but it adds more endpoints to preload, while the index endpoint is already preloaded and feels like a good place for some global uneditable settings. |
|
The index is available to every consumer, and that's one reason Core recommends exposing only a small amount of data about the site. I don't see how default template types and template part areas are relative in this context. IIRC, the only reason we made Logo/Icon available was the lack of proper read capabilities for those settings. The read-only block editor settings REST API endpoint has been mentioned several times. I can find the PRs now, but I remember the most recent discussion between @youknowriad and @TimothyBJacobs. Maybe it's time to invest in this long-term solution. |
Yes, it's a mobile specific endpoint basically only available in the Gutenberg plugin for now. That said, what I want to avoid is an "editor settings" endpoint personally. I don't see these things as "editor" settings, they are "WordPress settings" for me. |
|
default template and template parts are just titles and descriptions, so being available to everyone is not really an issue IMO but I can see the point. So what would be the ideal solution knowing:
Can we add these to |
|
I also want us to avoid getting stuck here, so let's think about a path forward that everyone can agree with. |
Then, maybe we can improve the existing P.S. Just saw the latest comments; it seems we're on the same page :)
100%. |
|
I would love some agreement on the direction here before we actually lose time bike shedding and implementing things that won't be agreed upon. So would love input from @TimothyBJacobs and others. The existing PR you shared @Mamaduka seems about settings that are saved to "site options". As I explained above, this is not the case for the settings that this PR is considering. These are just php filterable static values. The user doesn't update these. So if we are to add these to the "settings" endpoint, they can only be added adhoc if I'm not wrong. not need for generic settings registration or things like that. We could just hard code them in the endpoints no? |
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 moves the
defaultTemplateTypes,defaultTemplatePartAreasto thegetEntityRecord( 'root', '__unstableBase' ) endpoint.Given that the impacted selectors aren't specific to the
editorstore, I decided to deprecate them all. I couldn't find a suitable location to ensure accessibility across different packages, leading to some duplicated code. Should we create a@wordpress/templatepackage (similar to@wordpress/patterns) to house this logic? Create a dedicated package could help us with #65390 too.Why
This PR is necessary to remove the
core/editordependency across the application. In the specific, this refactor is needed to allow the packagefieldsto have access to thedefaultTemplateTypes,defaultTemplatePartAreaswithout that editor settings are initialized. More details here: #65390 (comment)Testing Instructions
Visit the Site Editor.
On the sidebar click on
Pattern.Click on
All template parts.Ensure that all the patterns are visible (you should see
HeaderandFooterlabels too)On the top right, click on
Add New Pattern.Create a new template part
Ensure that the template part is created correctly.
Visit the Site Editor.
Visit the
Blog Home.Ensure that the right panel under the template section renders the right data:
Add a block into the footer.
Click save.
Ensure that
are you ready to saveshows the right data:Testing Instructions for Keyboard
Screenshots or screencast