Patterns: Add My patterns back to post editor inserter categories #54767
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
|
Size Change: +201 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
glendaviesnz
changed the title
glendaviesnz
added
[Type] Bug
There was a problem hiding this comment.
Thanks for working on this @glendaviesnz 👍
The patterns displayed in the "My patterns" category worked as expected during a quick test.
The source filters may need some tweaking. The decision on the pattern filtering UI updates was that it didn't make much sense to have filter options enabled that we know wouldn't return results. That same logic should then be applied to the source filters for this category.
What do you think?
Screen.Recording.2023-09-25.at.12.38.02.pm.mp4
packages/block-editor/src/components/inserter/block-patterns-tab.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Aaron Robertshaw <[email protected]>
Good point, see what you think of this change. I will update the PR description. |
glendaviesnz
commented
Sep 25, 2023
| export function BlockPatternsSyncFilter( { | ||
| setPatternSyncFilter, | ||
| setPatternSourceFilter, | ||
| patternSyncFilter, | ||
| patternSourceFilter, | ||
| scrollContainerRef, | ||
| category, | ||
| } ) { |
There was a problem hiding this comment.
My thinking below is that we don't want to explicitly update the parent components patternSourceFilter state if the user switches to My patterns as otherwise it is then set to this filter when they switch to another category, but it is still good to show the User option as selected I think. So deriving it from the props here achieves this and avoids them being confused if they switch from My patterns to another category without themselves having explicitly chosen the user filter.
There was a problem hiding this comment.
have added a comment to source to explain this.
kevin940726
reviewed
Sep 25, 2023
There was a problem hiding this comment.
Looks good! One thing I noticed though, is that if you set the source filter in another category like "All patterns" to "Theme" then switch to "My patterns", it will show no results.
I wonder if we should reset all the filters when we switch categories. I think either way makes sense, but maybe resetting is less confusing? 🤔
It's especially visible in "My patterns" though that all the other options are disabled.
There was a problem hiding this comment.
Thanks for the iterations on this.
The source filters still aren't quite right. If you select a non-user source filter and then switch to the "My patterns" category, you'll get no results as the non-user source filter is still applied.
Screen.Recording.2023-09-25.at.3.32.15.pm.mp4
The latest inline comment could also be clearer by specifying who "they" are. It can be inferred by the user source filter but following the logic of all the patterns stuff is enough to have to think about for someone new coming to the code.
|
Just did #54767 (review) in 54b7c09. LMK if it solves the issue for you too! Another thing I noticed unrelated to this PR is that we need to reset the page after switching filters too. I'll put up a separate PR for this. |
Thanks, you were too quick 🙂 I'll pull the latest and re-review. |
Updated to say user instead of they |
|
It is a little bit annoying that you lose the filter on every category change now - but maybe this is good enough for 16.7 and we can refine later? |
I think it depends on what the intent of the filter is and where it is placed. If it were put in the tabs list then I would say it should retain the selected options, but if it's placed in the panel I would expect it to reset after changing to a new panel. But yeah like I said, I guess either way makes sense? 🤔 I don't really have a preference here. I think the other solution can easily be done with minimal effort too. |
Could we keep track of the original filter to restore it when switching away from the my patterns category. Not sure how beneficial that is in light of the extra complexity 🤷 |
@kevin940726 I don't have a strong opinion either way at this point. I think we could afford to merge it as is and see what the feedback is during beta, happy for you and @aaronrobertshaw to make a call on it if you want to get it merged tonight. |
There was a problem hiding this comment.
I've retested this and 54b7c09 does prevent the issue where previous filter selections were retained resulting in no results.
I'll hold off on a ✅ until we have consensus around the resetting of filters though.
Screen.Recording.2023-09-25.at.3.44.45.pm.mp4
aaronrobertshaw
approved these changes
Sep 25, 2023
There was a problem hiding this comment.
After discussing this more, we'll proceed with the current approach.
@kevin940726 makes a valid point about the location of the filter controls and how that may inform users. We'll definitely need to monitor feedback around this behaviour, but for now let's merge.
A fix for resetting the pagination is currently being worked on as a separate follow-up.
This is done in #54774. |
mikachan
pushed a commit
that referenced
this pull request
Sep 25, 2023
…4767) Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Kai Hao <[email protected]>
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 924d35c |
mikachan
removed
the
Backport to Gutenberg RC
This was referenced Oct 11, 2023
17 tasks
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?
Reinstates the "My patterns" category in the post editor's inserter as an easy place for users to see all their custom patterns at once.
Why?
Feedback has been provided that this category was missed after the addition of custom user-created pattern categories removed it in favour of an "All patterns" category.
How?
Testing Instructions
My patternscategory and that it displays the user created pattersnMy patternscategory also works in the patterns explorer modalMy patternscategory is selected that the directory and theme source filters are disabled as it doesn't make sense to be able to enable these in this category tabScreenshots or screencast
patterns-disable.mp4