-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Allow blocks with parents in the woocommerce
namespace to be rendered as inner blocks
#48543
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
Size Change: 0 B Total Size: 2.49 MB |
08fc9a3
to
b106b31
Compare
Hi @mikejolley, @senadir, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
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 got your testing steps to work, but there are some improvements we can make to the cache. I left comments below.
@mikejolley I also added PHP unit tests for this. |
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.
Thanks for making those improvements @opr
Happy with this so pre-approving. However the CI linting rules are not happy with the tests so make sure to appease the CI before merging.
44c50b2
to
6d833fb
Compare
6d833fb
to
622584f
Compare
return $this->registered_blocks_with_woocommerce_parents; | ||
} | ||
|
||
$registered_blocks = \WP_Block_Type_Registry::get_instance()->get_all_registered(); |
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 all blocks are PHP registered, but this should work.
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.
Approving this, I'd wish we don't really need this for now, but I can't think of something else.
…ed as inner blocks (#48543)
…ed as inner blocks (#48543)
Changes proposed in this Pull Request:
woocommerce
namespace. Do this oninit
.data-block-name
added, consider if their parent is in thewoocommerce
namespace (cached above)Closes #46264
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
@woocommerce/extend-cart-checkout-block test-block
package.wp-content/plugins
and runnpx @wordpress/create-block -t @woocommerce/extend-cart-checkout-block test-block
.src/js/checkout-newsletter-subscription/block.json
) so it is:npm run start
)Newsletter Subscription!
block.block.json
settings and set thelock.remove
property totrue
.Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment