Patterns: merge unsynced into inserter patterns tab and add paging and filtering#54007
Merged
glendaviesnz merged 61 commits intoSep 14, 2023
Conversation
glendaviesnz
added
[Type] Enhancement
This was referenced Aug 29, 2023
|
Size Change: +1.79 kB (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 8b94003. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6178947022
|
glendaviesnz
changed the title
glendaviesnz
force-pushed
the
update/block-editor-inserter-merge-patterns
branch
from
dc0b60e to
f432bc2
Compare
glendaviesnz
force-pushed
the
update/block-editor-inserter-merge-patterns
branch
from
f432bc2 to
fda3f16
Compare
glendaviesnz
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
glendaviesnz
changed the title
Contributor
|
@glendaviesnz one issue I noticed, unsure if it's related to this PR: If you click the "Explore all patterns" button, the modal doesn't include any patterns. |
Contributor
|
@glendaviesnz a final question: is the pagination UI part of this PR? It looks a little rough. I can adjust that tomorrow if need be. |
Contributor
Author
The modal shows patterns for me @jameskoster: explore-all.mp4Do you see any errors in the console when you load the modal? |
Contributor
Author
yeh, and it turned out that opening that panel by default was not great for keyboard accessibility, so have added back the All category for now and differentiated the |
Contributor
Author
or if we get this tested and signed off today it will be merged into the main categories PR and paging could be adjusted there. |
kevin940726
approved these changes
Sep 14, 2023
kevin940726
left a comment
kevin940726
left a comment
Member
There was a problem hiding this comment.
Looking pretty good! Just a couple of minor suggestions and none are blockers! 💯
Comment on lines
+32
to
+43
| // If the sync filter changes, we need to select the "All" category to avoid | ||
| // showing a confusing no results screen. | ||
| useEffect( () => { | ||
| if ( patternSyncFilter && patternSyncFilter !== previousSyncFilter ) { | ||
| setSelectedCategory( initialCategory?.name ); | ||
| } | ||
| }, [ | ||
| patternSyncFilter, | ||
| previousSyncFilter, | ||
| patternSourceFilter, | ||
| initialCategory?.name, | ||
| ] ); |
Member
There was a problem hiding this comment.
Nit: Could we rewrite this without the useEffect? Or even better if we could pass setSelectedCategory down and call it when the sync filter changes.
Contributor
Author
There was a problem hiding this comment.
yeh it would be nice to do that, but had a look and plumbing through the onCategorySelect method from modal.js file and using that causes the whole component tree to rerender and the modal gets closed. But would be great to refactor this component tree at some point to allow away for these changes to be handled in the relevant event handlers
Comment on lines
+321
to
+332
| // If the sync filter changes, we need to select the "All" category to avoid | ||
| // showing a confusing no results screen. | ||
| useEffect( () => { | ||
| if ( patternSyncFilter && patternSyncFilter !== previousSyncFilter ) { | ||
| onSelectCategory( allPatternsCategory, patternSourceFilter ); | ||
| } | ||
| }, [ | ||
| patternSyncFilter, | ||
| previousSyncFilter, | ||
| onSelectCategory, | ||
| patternSourceFilter, | ||
| ] ); |
Member
There was a problem hiding this comment.
Nit: Same here. Could we directly set the state in the render phase to avoid an extra render? Never mind if it's not possible though!
glendaviesnz
deleted the
update/block-editor-inserter-merge-patterns
branch
Contributor
Author
|
This was merged with some failing tests, but is going into a feature branch which needs rebasing to get the failing tests resolved, so failing tests will be sorted on that branch prior to merging to trunk. |
This was referenced Sep 24, 2023
This was referenced Oct 11, 2023
17 tasks
2 tasks
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
9 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?
Merges the synced patterns tab into the main patterns tab and adds an
All patternscategory and paging and filtering in the patterns inserter tab and patterns explorer.Why?
Currently it is confusing that synced patterns are in a separate tab, and also causes performance issues because patterns are not paged.
How?
This PR was part of a group of PRs that were merged back into #53835 before being merged into trunk. This PR was branched off #53933.
A full list of the PRs involved in this work can be found here.
Testing Instructions
All patternstab shows all patterns and that paging at the bottom of the tab worksMy patternssource filter is selected that aSync type filterappears at the top of the patterns list and works as expectedAllScreenshots or screencast
patterns-final.mp4