Tie zoom out availability to content post supports #66600
+60
−10
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
draganescu
added
[Type] Enhancement
draganescu
force-pushed
the
try/post-content-support
branch
from
3270935
to
1a09e97
Compare
fabiankaegy
reviewed
Oct 30, 2024
| global $wp_post_types; | ||
|
|
||
| foreach ( array_keys( $wp_post_types ) as $post_type ) { | ||
| if ( in_array( $post_type, array( 'post', 'page', 'wp_template' ), true ) ) { |
There was a problem hiding this comment.
I like the idea of this in general. But it seems odd that wp_template has a post type support called content.
In all the other places where we differentiate between wether something is content or now wp_template explicitly counts as non content
There was a problem hiding this comment.
Yes, the support type name is debatable b/c of this. It's also still unclear what is content and what is not ... Let's see what other reviewers think.
There was a problem hiding this comment.
What should the default be for custom post types that plugins might be creating? I.e. is the intention that we're gating the zoom out feature so that plugins will need to opt-in in order to support it?
(I don't have an opinion either way, just a thought as I looked over the PR 🙂)
There was a problem hiding this comment.
There are some kinds of post types where it doesn't make sense. We can make all post types support this by default and opt out only when a custom supports list is provided.
draganescu
force-pushed
the
try/post-content-support
branch
from
1a09e97
to
e8896b9
Compare
draganescu
requested review from
spacedmonkey,
ajitbohra,
juanmaguitar and
ndiego
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. |
draganescu
removed
the
[Type] Code Quality
Mamaduka
reviewed
Oct 30, 2024
Comment on lines
+1817
to
+1830
| /** | ||
| * Returns a post type support object on the current post | ||
| * | ||
| * @param {Object} state Global application state. | ||
| * | ||
| * @return {Object} The post type supports object. | ||
| */ | ||
| export const getPostTypeSupports = createRegistrySelector( | ||
| ( select ) => ( state ) => { | ||
| const currentPostType = getCurrentPostType( state ); | ||
| const postType = select( coreStore ).getPostType( currentPostType ); | ||
| return postType?.supports ?? {}; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Do we need to introduce a new public registry selector for the value the editor has been deriving forever? 😅
There was a problem hiding this comment.
I understand we should not add this selector, but what does "the editor has been deriving forever" mean? Do we have this data selectable already?
There was a problem hiding this comment.
Oh I see you mean maybe derrive it from the entity call
const postTypeSupportsComments = useSelect( ( select ) =>
postType
? !! select( coreStore ).getPostType( postType )?.supports.comments
: false
);
Well we do have a selector above for post title ... IDK!
There was a problem hiding this comment.
Components like PostTypeSupportCheck just duplicated the logic and derived value from the supports property.
P.S. I didn't meant this as snarky remark, sorry if it came out that way 🙇
|
Size Change: +47 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e8896b9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11594426519
|
2 tasks
youknowriad
reviewed
Nov 4, 2024
| @@ -555,6 +555,18 @@ _Returns_ | |||
|
|
|||
| - `string|undefined`: The post type label if available, otherwise undefined. | |||
|
|
|||
| ### getPostTypeSupports | |||
There was a problem hiding this comment.
Why is this a selector in the editor package?
There was a problem hiding this comment.
I have not thought about putting it here on purpose, it just came naturally after the getPostTypeLabel selector 🤷🏻
There was a problem hiding this comment.
I see, I was not aware of getPostTypeLabel. I agree that it's similar but it seems to me that neither should exist to be honest (especially in the editor package).
There was a problem hiding this comment.
Why tho? From the editor package readme:
Having an awareness of the concept of a WordPress post, it associates the loading and saving mechanism of the value representing blocks to a post and its content.
Since it knows of WordPress posts what is wrong with sugar selectors like these?
There was a problem hiding this comment.
I think we should avoid "convenient" selectors or shortcuts like that. There's more detailed reasoning here https://make.wordpress.org/core/2024/09/12/gutenberg-development-practices-and-common-pitfalls/
If we allow that, we'll never stop adding shortcuts and APIs everywhere. We just use core-data for these directly.
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.
Follow up to #65732
Currently there is a hardcoded check for certain post types to see if we should offer users the zoomed out view or not. Ideally this decision should be based on an ability of the currently edited post type.
This PR adds a new post support to a few core post types, a post support named "content".
It then checks if the currently edited post has this new content support.
Context: