-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix TypeError when duplicating uncategorized theme patterns #66889
Conversation
Only try to match the pattern categories and get the label if the theme pattern uses the `categories` parameter. Otherwise there will be a TypeError when using the filter.
Size Change: 0 B Total Size: 1.82 MB ℹ️ View Unchanged
|
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. |
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.
Left a small comment, but this looks good. Thank you!
The array fallback is not needed inside getTermLabels, because getTermLabels is used as the value for defaultCategories, which is an empty array by default when it is used inside the function CreatePatternModalContents.
I briefly looked into adding a Playwright test for this but I was not able to figure out if there was a way to test a theme provided pattern. |
Oh, or I am supposed to get theme patterns by |
So I think the test should wait until TT5 is released, so that it can be added to the gutenberg-test-themes. |
Flaky tests detected in 8e64ae6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11775413937
|
For tests we're using the empty theme, where we can add stuff, like templates, patterns etc.. Did you try adding one there? |
I did not: I am torn about that because then it wont be empty 😅 |
I get it 😄 , but it's not entirely empty right now. Anyway.. This fix is a JS pretty strait forward one, and I believe it would be fine not to add a test. |
What Why & How
When a pattern is duplicated, there is a modal with an input field that contains the labels of the pattern categories:
the information about the category is passed on to the new pattern.
The function that gets the labels compares the category from the theme pattern with the existing registered pattern categories, to make sure that the category exists and is displayed with the correct label.
However, the code did not take into account that theme patterns can be registered without the optional
categories
parameter.This PR makes a small change to the filter and mapping using optional chaining, to avoid a TypeError when the
categories
parameter is not used.Closes #66888
Testing Instructions
Activate a theme that registers a pattern without the
categories
parameter. This can be a classic theme or a block theme, for example Twenty Twenty-Five.Open the Patterns screen from Appearance > Patterns or from Appearance > Editor > Patterns.
Locate the pattern that does not have a category.
Select the option to duplicate the pattern.
Confirm that the modal with the information about the duplicated pattern opens and that there are no JavaScript errors in the browser console.
Click the button at the bottom of the modal to duplicate the pattern. Confirm that there are no issues related to the category of the new pattern.
Now, select a different pattern that has a single category or multiple categories, and duplicate it.
Confirm that the category labels show correctly in the modal and that there are no new issues/ regressions.
Click the button at the bottom of the modal to duplicate the pattern. Confirm that there are no issues related to the category of the new pattern.