Block Patterns tab: Handle receiving the category name as a string#68806
Closed
carolinan wants to merge 3 commits into
Closed
Block Patterns tab: Handle receiving the category name as a string#68806carolinan wants to merge 3 commits into
carolinan wants to merge 3 commits into
Conversation
|
Size Change: +35 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
Contributor
I tried some similar changes myself and what most bothered me is the case where "Starter content" does not exist. On trunk, due to happenstance, the "All" category is shown. On this branch, the blank pattern preview you mention shows up. (screenshot disclosed)
If we aren’t going to revert to the modal, I suppose this is acceptable. The modal however doesn’t have this problem because it only presents if/when the "Starter content" category is available.
What complicates the idea to catch missing/invalid categories is that it may be temporary. At least in the case of "core/starter-content", the category is always absent in the initial render and may or may not become available. I didn’t find a simple way to account for this. How long it takes to show up depends on network conditions. |
Contributor
Author
|
Even if the modal is used instead, there is still a problem with passing any category to the pattern preview in an unexpected format. |
Mamaduka
added
[Type] Bug
Mamaduka
reviewed
Jan 27, 2025
Comment on lines
82
to
84
| const [ selectedPatternCategory, setSelectedPatternCategory ] = useState( | ||
| __experimentalInitialCategory | ||
| ); |
Member
There was a problem hiding this comment.
The above logic can be inlined here.
diff --git packages/block-editor/src/components/inserter/menu.js packages/block-editor/src/components/inserter/menu.js
index f1e387c5a59..caec6adef78 100644
--- packages/block-editor/src/components/inserter/menu.js
+++ packages/block-editor/src/components/inserter/menu.js
@@ -68,9 +68,9 @@ function InserterMenu(
const [ filterValue, setFilterValue, delayedFilterValue ] =
useDebouncedInput( __experimentalFilterValue );
const [ hoveredItem, setHoveredItem ] = useState( null );
- const [ selectedPatternCategory, setSelectedPatternCategory ] = useState(
- __experimentalInitialCategory
- );
+ const [ selectedPatternCategory, setSelectedPatternCategory ] = useState( {
+ name: __experimentalInitialCategory,
+ } );
const [ patternFilter, setPatternFilter ] = useState( 'all' );
const [ selectedMediaCategory, setSelectedMediaCategory ] =
useState( null );
Contributor
Author
There was a problem hiding this comment.
I am not sure I understand. Should the check for the string format be removed? Right now in Gutenberg, the requesting components passes a string, or: I only found two places where a category name is passed at all. But the expected received format is still meant to be an object, no? So it needs to handle both string and object.
Member
There was a problem hiding this comment.
Currently, only selectedPatternCategory and selectedMediaCategory values are expected to be objects with the name property. The remaining code can use string value.
Inlining object as initial state value can simplify things, IMO.
const [ selectedPatternCategory, setSelectedPatternCategory ] = useState( {
name: __experimentalInitialCategory,
} );
Contributor
Author
There was a problem hiding this comment.
But it assumes __experimentalInitialCategory is always a string? If it is already an object it will be placed inside name
Contributor
Author
There was a problem hiding this comment.
If it is truly never and object, then shouldn't the block patterns tab and the preview be updated to use strings?
Member
There was a problem hiding this comment.
Correct. The __experimentalInitialCategory should always be a string, but pattern tabs will use an object internally because of how the Tabs component works.
P.S. This is probably unimportant now, considering we will revert the feature - #68736 (comment).
Edit: But we'll have to deal with this issue anyway, so let's keep this open.
Mamaduka
reviewed
Jan 27, 2025
Member
|
The missing fallback for invalid categories - I think this predates changes from #66836 and was only highlighted. While I would like to see it resolved, it isn't a blocker for this PR. |
carolinan
changed the title
Member
|
@carolinan, should we remove this from the WP 6.8 project board in favor of #69081? |
Contributor
Author
Ok |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 started as a bug fix related to the page creation flow, where the
starter contentpattern category was meant to show when you create a new page.But it turned out the problem was not only with the patterns that are meant to show on page creation, but with passing any category name to the block patterns tab as a string.
That is why there is no separate issue opened for this PR.
Partial for #68736
Alternative to #68801
In the pattern category preview, the expected format of the category is an object with a
nameandlabelparameter.However, in the components that pass the category to the previewer, the category is a string.
The
ZoomOutModeInserterButtonpasses the value "all" -and this is not recognized as a category by the previewer.The previewer is expecting the name parameter with the value "allPatterns":
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/inserter/block-patterns-tab/utils.js#L18
The
StartPageOptionspasses the value "core/starter-content" and the previewer is expecting thenameparameter with this value.So what this PR does is:
name, so that the previewer receives the correct identifier for the categoryTesting Instructions
Activate a block theme that has starter content patterns, for example Twenty Twenty-Five.
Create a new page and observe which pattern category that is shown in the inserter sidebar.
Edit a template and observe which pattern category that is shown when adding a pattern in zoom out mode.