Patterns: fix error with react list key with new custom patterns list in inserter #51877
+73
−34
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
glendaviesnz
added
[Type] Bug
glendaviesnz
added
the
Backport to Gutenberg RC
This was referenced Jun 25, 2023
Closed
|
Size Change: +141 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Jun 26, 2023
There was a problem hiding this comment.
Thanks for the quick fix on this @glendaviesnz 🚀
For some reason, I can't replicate receiving the console error described in #51873 (either on trunk or this branch).
However, I could replicate the display of duplicate custom patterns in the block inserter on trunk. This PR fixes that.
Looking at the code, I believe the change should fix both issues. I'm happy to approve this change though it will be good if @t-hamano can confirm the errors he reported have been resolved.
|
Flaky tests detected in 87a8812. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5375175426
|
|
Thanks for the PR, @glendaviesnz! ✅ For #51872, I have confirmed that this PR fixes the problem. ❌ For #51873, there still seems to be a problem. You may need more than one custom pattern to reproduce this issue. It appears that similar changes may need to be made to the key of this component. Here are the complete steps to reproduce the issue on this PR: ac49ef916e6c9a5593b4a0416a1c042f.mp4 |
t-hamano
approved these changes
Jun 26, 2023
There was a problem hiding this comment.
LGTM!
However, I am concerned that I was the only one who could reproduce the #51873 problem. I deleted all Docker resources once and tried again in a WordPress 6.2.2 environment. The warning error does not go away unless there are changes made by this PR. I am testing this PR with the npm run dev command.
Perhaps SCRIPT_DEBUG is turned off for some reason? 🤔
🤦 yep, that was the problem - could replicate on trunk when that was switched back on. |
ramonjd
added
the
Backport to WP 6.7 Beta/RC
* Add custom patterns to pattern explorer * show custom patterns in the patterns explorer dialog * remove changes from 51877 * Fix up use of async lists * remove a bit of code duplication by adding a new hook * add 51877 fix back to make testing easier
aaronrobertshaw
approved these changes
Jun 26, 2023
There was a problem hiding this comment.
🤦 yep, that was the problem
Ditto. I cut a fresh local environment this morning and missed setting SCRIPT_DEBUG as well.
This is testing as advertised for me.
✅ Can replicate both the console error and duplication of patterns on trunk
✅ Confirmed this PR fixes both the console error and pattern duplication
✅ Patterns now also correctly appear in the pattern explorer as per #51889
packages/block-editor/src/components/block-patterns-list/index.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Jun 26, 2023
| @@ -135,9 +135,12 @@ function BlockPatternList( { | |||
| > | |||
| { blockPatterns.map( ( pattern ) => { | |||
| const isShown = shownPatterns.includes( pattern ); | |||
| // User added unsynced patterns do not have a unique name so we use the id instead. | |||
| const key = | |||
| pattern.name === 'core/block' ? pattern.id : pattern.name; | |||
There was a problem hiding this comment.
I think this component is BlockPatternsList is supposed to render patterns and patterns are expected to have unique names, so I think a better fix would be on the "caller" of this component, in the place where we transform "reusable blocks" to "patterns".
There was a problem hiding this comment.
I'm guessing that means useUnsyncedPatterns
There was a problem hiding this comment.
I included this change in this alternative PR
youknowriad
reviewed
Jun 26, 2023
| onInsert, | ||
| withBlocks | ||
| ) { | ||
| const [ unsyncedPatterns ] = useBlockTypesState( |
There was a problem hiding this comment.
Isn't it a bit weird, to have useBlockTypesState responsible for returning the "unsynced" patterns? Should we move these to usePatternsState instead? which would probably ensure that we don't need a special case for unsynced pattern and the useUnsyncedPatterns hook is not needed anymore.
There was a problem hiding this comment.
This makes sense, but is a reasonable refactor, so have done it as a separate PR here.
|
Going to close for now as I think #51947 is a superior approach. |
ramonjd
removed
Backport to WP 6.7 Beta/RC
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?
Fixes console error when browsing
Custom patternsin the inserter patterns panelWhy?
Fixes: #51873 and #51872
How?
If the pattern is a
core/blocksets the React list key toidinstead ofnameTesting Instructions
For #51873
For #51872
Screenshots or screencast
Before:
10d517cb75e3b47b76755c52c24bc53c.mp4
c22f1fcfc097fa80150a073597e14f6f.mp4
After:
patterns-bug.mp4
duplicate.mp4