Populate block context with inherited post type from template slug #65062
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
|
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. |
|
Size Change: +653 B (+0.04%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Sep 4, 2024
Comment on lines
182
to
185
| postTypes: | ||
| getPostTypes( { per_page: -1 } )?.map( | ||
| ( entity ) => entity.slug | ||
| ) || [], |
There was a problem hiding this comment.
You should avoid map/filter operations for returned values from mapSelect. They will always return new reference and force component to re-render even if derived value is the same.
P.S. You should see warning in console for similar cases in dev mode. See #53666.
There was a problem hiding this comment.
Thanks for the info! I wasn't aware of that 🙂 I have just changed it in this commit.
Not related to this pull request, but do you think it could be an issue also here? I'm happy to create another PR to change that.
There was a problem hiding this comment.
Thank you, @SantosGuillamot!
Not related to this pull request, but do you think it could be an issue also here? I'm happy to create another PR to change that.
That one is okay. It will pass a shallow equality check since the filteredPostTypes values will have the same reference. It's somewhat undocumented and a last-resort hack.
It usually depends on the case; check for a warning in the DevTools if you're unsure.
// ❌ will fail shallow equality since the `postType` property has a new reference on each call.
return {
postTypes: types.filter( () => /*--/* );
}
// ✅ will pass shallow equality since returned object properties have the same object reference
return types.filter( () => /*--/* );
There was a problem hiding this comment.
I tested this using the single template and the post type is being successfully added to the context 👍 I think we could merge this separately or revise #64072 with this approach.
SantosGuillamot
changed the title
cbravobernal
approved these changes
Sep 11, 2024
| return { | ||
| editorSettings: getEditorSettings(), | ||
| isReady: __unstableIsEditorReady(), | ||
| mode: getRenderingMode(), | ||
| selection: getEditorSelection(), | ||
| postTypes: getPostTypes( { per_page: -1 } ), |
There was a problem hiding this comment.
This is the only concern I may have. Sites with tons of postTypes could make the site editor slower.
There was a problem hiding this comment.
This needs to be further investigated. We should see the results soon at https://codehealth.vercel.app/project/gutenberg.
In this PR on CI, I see the following metrics:
firstBlock is substantially slower for both editors and needs to be double checked on trunk.
There was a problem hiding this comment.
I'm wondering if we have the following abstracted:
const postTypes = await apiFetch( {
path: '/wp/v2/types?context=view',
} );It's preloaded on the server, and it's enough to collect all post types and their slugs. Maybe it's exactly what getPostTypes does.
There was a problem hiding this comment.
The performance regressions was confirmed by @SantosGuillamot in #65705 (comment). @WordPress/gutenberg-core, I would appreciate help during the beta phase to address this issue.
There was a problem hiding this comment.
This issue should be better after merging this pull request. It is already reflected in the first results from https://codehealth.vercel.app/project/gutenberg/site-editor-firstBlock:
This was referenced Sep 26, 2024
This was referenced Oct 14, 2024
2 tasks
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?
Try to populate the block context in those use cases where we can inherit it from the template slug following the template hierarchy.
Why?
In some use cases, where the template is linked to a specific post type, I believe we can be sure the post type it relates to. If that's the case, it could make sense to populate the block context with that information.
How?
In the
BlockContextProvider, I'm checking if it iswp_templateand consider these scenarios:page, we can assume it is a page.slug-{postType}, we check if the{postType}exists in the registered post types and, if it does, we can assume that template is specific to that post type.Testing Instructions