-
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
Patterns: Add categories to user patterns, and allow filtering by these in site and post editor #53835
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.4/block-patterns.php |
Size Change: +2.84 kB (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Flaky tests detected in 85f4c5e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6194045390
|
6c97291
to
e6a30e7
Compare
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.
Nice work here @glendaviesnz 👍
This is looking good and tests as advertised for me.
✅ Pattern creation via the post editor works
✅ Creation via the site editor also works
✅ Pattern categories are correctly created or found, and assigned
✅ Post type admin page reflects the categories as they were assigned
Once we merge in the PRs built on this, we can give it a further holistic test before merging to trunk.
I'm looking forward to seeing all the pattern categorization in the wild!
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.
All worked great 👍 Part of me wanted to see the categories on the patterns view in the site editor.
✅ In the post editor add a block and then in the block toolbar overflow select the Create pattern option
✅ Add categories for the pattern in the create pattern model and add the pattern
✅ Go to /wp-admin/edit.php?post_type=wp_block and check that the categories are attached to the new pattern
✅ Try adding another pattern and try a mix of adding existing and new category names and make sure they are all saved
✅ Also try adding a new pattern from the site editor Patterns page
Screen.Recording.2023-09-13.at.10.09.50.AM.mp4
e291597
to
81876cd
Compare
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.
First things first, huge effort wrangling pattern categories @glendaviesnz 💪
I've started testing the overall feature and it is coming together nicely!
Unfortunately, I'm out of time today, so haven't yet gotten to thoroughly test all angles, or review the code completely, but thought I could share a couple of issues I've run into so far.
- If you create a new pattern via the Patterns page's "Create pattern" menu item, then navigate back to the main Patterns page. You'll end up with the old
my-patterns
category selected even though that is no longer an option in the sidebar screen. - There's an inconsistent error thrown when searching patterns that crashes the page.
https://github.com/WordPress/gutenberg/assets/60436221/dd6a4a9d-c5b7-474f-8c62-ae9d0741b4b4 - In the post editor's pattern explorer modal, if you change the sync type filter there the modal closes unexpectedly.
Screen.Recording.2023-09-14.at.8.47.23.pm.mp4
Some other random thoughts were:
- It felt a bit odd to be changing the sync type filter in secondary panel in the inserter and then have the categories on the first panel change. It seemed like the sync type filter is visually scoped to the panel it is in.
- We should probably add pattern categories information to the pattern details screen when viewing a pattern
- Should we be enforcing that a pattern is named?
- Nit: There seems to be a 2px difference in height between the name input and the token input in the create pattern modal form.
I'll keep testing tomorrow and help out in any way I can.
It is looking pretty good though, I'm excited to see the feature land!
These two were related. The idea with updating the categories was that currently the default was not to show categories that were not populated. However I agree the resulting UX is a little odd, and in fact it probably works better to just show 0 results for the cases where a sync filter change means the category is not populated. I have a PR here that takes this approach, and it also fixes the issues with the modal randomly closing which was caused by a rerender triggered by the category updates. |
Fixed here. |
Looks like the console error might have been introduced by #54450 |
Confirmed. This doesn't appear to have been caused during the conflict resolution but introduced via #54450. A separate fix for that issue is probably better for historical purposes. So I don't think it is a blocker for this PR. |
Not a big deal, just adding it here coz it fresh. When trying to add a category that already exists, the API returns a 400 Bad Request error: 2023-09-18.11.16.13.mp4If this is a problem, maybe we could fetch the taxonomy records and check if it exists before adding it, similar to how post taxonomies do it? gutenberg/packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js Line 277 in 900439e
|
@ramonjd that approach was copied from the post taxonomies flat-term-selector , so I assumed it was a legit approach, but happy to review this approach if you have concerns about it. |
No worries, I was just flagging it. Thanks for the explainer. 👍🏻 |
👋 This PR seems to have caused a large increase (58%) in the editor's typing performance metric: From https://www.codevitals.run/project/gutenberg. cc @youknowriad |
@@ -68,7 +80,7 @@ const usePatternsState = ( onInsert, rootClientId ) => { | |||
[ createSuccessNotice, onInsert ] | |||
); | |||
|
|||
return [ patterns, allCategories, onClickPattern ]; | |||
return { patterns, allCategories, onClickPattern }; |
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.
Was this change necessary at all? Now it's not consistent anymore with useBlocksState.
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 object return makes for a nicer API, eg. avoids the likes of const [ patterns, , onClickPattern ] =
. An array return makes sense for hooks like useState where you are always going to use both values and want to easily give them context-specific names, but not so much in instances like this hook.
I don't have a strong opinion on this though. If keeping it consistent with useBlocksState
is thought to be more important then there is a revert PR here.
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.
Not sure exactly what caused the regression but it's meaningful indeed. Worth looking into.
Also the PR is a bit too big. I wonder if it could have shipped in smaller bits.
/** | ||
* Retrieve the registered user pattern categories. | ||
* | ||
* @param state Data state. | ||
* | ||
* @return User patterns category array and map keyed by id. | ||
*/ | ||
|
||
export function getUserPatternCategories( | ||
state: State | ||
): UserPatternCategories { | ||
const patternCategoriesMap = new Map< number, UserPatternCategory >(); | ||
state.userPatternCategories?.forEach( | ||
( userCategory: UserPatternCategory ) => | ||
patternCategoriesMap.set( userCategory.id, userCategory ) | ||
); | ||
return { | ||
patternCategories: state.userPatternCategories, | ||
patternCategoriesMap, | ||
}; | ||
} |
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.
This needs to be memorized. The useSelect
noticed from this screenshot is caused by this selector - #53835 (comment).
Aside from this, is there a reason we're creating a map based on the state inside a selector? The task is better suited for consumers, in my opinion.
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 doesn't look like this selector was causing that screenshot error as that error was there on trunk prior to this merge, and still there after fixing this selector - we will look at that separately.
This selector was causing the useBlockEditorSettings useMemo return value to be rerun with each call though, which happens on each keypress, so have moved the map into the consumers as suggested. While investigating this I also discovered another change which was causing this useMemo to be reevaluated with every call of the hook. This has been fixed here.
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.
#54580 seems to have resolved the typing issue: thanks @WunderBart for highlighting this, and @Mamaduka for the pointer to that selector. |
Thanks for the follow-ups, @glendaviesnz! |
What?
Add the ability to assign categories to user created patterns.
To make reviewing easier separate PRs will be created off this branch for:
Why?
Currently users have no way of organising their synced and unsynced patterns.
How?
Uses the recently added
wp_patterns_category
custom taxonomy, and adds a taxonomy input box to the create pattern modal.Testing Instructions
Create pattern
option/wp-admin/edit.php?post_type=wp_block
and check that the categories are attached to the new patternSite editor testing instructions
Footer
and make sure it displays along with the theme Footer patterns (be aware that although the TT3 theme footer patterns are listed underFooters
the category slug is actuallyfooter
so the user category name needs to match this).Post Editor testing
All patterns
tab shows all patterns and that paging at the bottom of the tab worksMy patterns
source filter is selected that aSync type filter
appears at the top of the patterns list and works as expectedAll
Screenshots or screencast
patterns-cats.mp4