-
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
Call variation through callback so it's only loaded when needed - in support of trac 59969 #56952
Call variation through callback so it's only loaded when needed - in support of trac 59969 #56952
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @kt-12! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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.
One nitpic, otherwise looks good.
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.
Great work @kt-12 .
I can merge this once the automated tests pass.
I believe this PR won't need a manual PHP backport. |
@Mamaduka doesn't the change in |
@tellthemachines, no, it's a shim for a new property to work with older WP versions. It can be removed after the required version is 6.5. |
✅ I updated the PHP Sync Tracking Issue to note this PR does not require a backport. |
@@ -115,6 +115,8 @@ | |||
<element value="apply_block_core_search_border_style"/> | |||
<element value="apply_block_core_search_border_styles"/> | |||
<element value="build_dropdown_script_block_core_categories"/> | |||
<element value="build_navigation_link_block_variations"/> |
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.
@kt-12, I appreciate your efforts with the ignore list. However, these functions weren't part of the initial setup and don't pose any backward compatibility issues if we stick to the naming conventions enforced by the sniff.
For example, they could be renamed to block_core_template_part_build_area_variations()
and block_core_post_terms_build_variations()
respectively. Could you please rename them?
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.
Here is the follow-up #58538.
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 names I suggested for these functions were not exactly correct, but the idea was to adhere to the coding standard. Thanks for fixing it, @Mamaduka.
I have approved your PR.
What?
This PR will help build variations only when we need to load them instead of during resignation at the
init
hook.Core PR with detailed performance analysis ->
WordPress/wordpress-develop#5718
Why?
Currently, we run a performance-heavy build variation function for some of the blocks in
init
hook, which causes it to load even on the frontend where it is not needed.How?
In this PR we register only the callback during block registration and build the variation only when they are needed.
Note: This PR provides around ~9% improvement to the frontend load time but it has no improvement for the editor site. However, we have found some way by which similar performance benefits could be obtained at the editor side - WordPress/wordpress-develop#5718 (comment)
Testing Instructions
Test it for the TT4 theme.
wp.blocks.getBlockType('core/post-terms')
core/navigation-link
,core/template-part
Testing Instructions for Keyboard
Screenshots or screencast