-
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
BlockPatternsList: use the Async component #66744
Conversation
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. |
Size Change: -298 B (-0.02%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
@@ -86,6 +87,9 @@ export function BlockPreview( { | |||
); | |||
} | |||
|
|||
BlockPreview = memo( BlockPreview ); |
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.
Minor point, but instead of reassigning values (which is more fragile, since now we need to be careful to place the assignment of BlockPreview.Async
after it), we should fix this at the source:
export const BlockPreview = memo(
( {
...
} ) => {
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.
I went for a rename in ca7fa6d because I'm not a fan of the extra indentation this creates. :)
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.
Overall this makes sense and tests well. Feel free to wait for a more experienced opinion, but LGTM.
Thanks for the review! You are an experienced opinion! I feel confident about this so I'll merge, we can always tweak or revert if needed. |
Co-authored-by: ellatrix <[email protected]> Co-authored-by: mcsf <[email protected]>
What?
Since #60425, we use an Async wrapper component around the BlockPreview components for templates in DataViews. This PR uses the same approach for
BlockPatternsList
, which currently usesuseAsyncList
.Why?
State is managed more down the tree, which is easier from a dev ex perspective, but also more performant since it doesn't cause the whole list to re-render.
How?
I've moved Async to the BlockPreview component so it can be re-used.
Testing Instructions
There's quite a few places where we use BlockPatternsList, most notably in the patterns inserter, but also in the patterns explorer modal (accessed from the inserter), the template swap function, and the starter content modal.
✅ The Post Editor > loadPatterns metric does not regress.
Testing Instructions for Keyboard
Screenshots or screencast