-
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
TemplateContentPanel: Don't show content blocks that are in a Query Loop #63732
Conversation
Size Change: +115 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
export const getPostBlocksByName = createRegistrySelector( | ||
( select ) => ( state, blockNames ) => { |
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 agree that making this a private selector makes sense for now.
We should also memoize the selector, but blockNames
being an array might make that more challenging. Example: https://github.com/WordPress/gutenberg/pull/63732/files#diff-cf33f4589314d4a81e5a821d1433b83521dca9ad19d58a4b5ba1687e314b9558R35-R37
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.
Hmm yeah this is tricky to get right. Please double check f2fea66 for me 😅
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 wonder if making a selector variadic function would work here.
Something like:
export const getPostBlocksByName = ( state, ...blockNames );
getPostBlocksByName( ...contentOnlyBlockTypes );
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 don't want to stray far from getBlocksByName
though.
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.
It's not a block for this PR. It's just an idea; I'm happy to give it a try separately.
Memoizing similar selector is always a plain 😅
ba5175e
to
4ef7332
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. |
'editor.postContentBlockTypes', | ||
POST_CONTENT_BLOCK_TYPES | ||
), | ||
'core/template-part', |
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.
The core/template-part
was removed from the const because we don't want extenders to remove it, correct?
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 was thinking along the lines of:
editor.postContentBlockType
allows extenders to control which block types we consider to be "post content".- We're enabling selecting template parts via the same mechanism but it's not "post content".
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.
Thanks, @noisysocks!
The changes test well for me and fix the issue ✅
Fixes #63613.
When editing a page in the site editor, we populate the Content panel in the sidebar with links to the content blocks that are editable.
Currently we blindly look for any content blocks: Post Title, Featured Image, Post Content.
This has the downside of catching blocks that appear nested within Post Content or within a Query Loop.
For example, you should only see Content once here:
There was already logic for fixing exactly this in
DisableNonPageContentBlocks
, so I've moved that logic into a selector that we can use in multiple places.Now we correctly only show the top-level content blocks:
Testing