-
Notifications
You must be signed in to change notification settings - Fork 21
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
Product Block Editor: Add the fundamental setup for the custom product blocks and map Select
and BooleanSelect
inputs to the custom select field block
#2164
Conversation
ad893dd
to
ca54755
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/support-product-block-editor #2164 +/- ##
========================================================================
+ Coverage 60.6% 60.8% +0.2%
- Complexity 4144 4151 +7
========================================================================
Files 453 453
Lines 17785 17824 +39
========================================================================
+ Hits 10777 10843 +66
+ Misses 7008 6981 -27
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Select
and BooleanSelect
inputs to the custom block of the select fieldSelect
and BooleanSelect
inputs to the custom select field block
917931b
to
220dd41
Compare
220dd41
to
6f17074
Compare
…e to get the support of the newer WC packages.
…condition_callback` to be null.
9a81d84
to
0408038
Compare
6f17074
to
a308eb7
Compare
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.
✅ LGTM. Thanks for working on this.
- I tested changing values in Products Single, Variable and Variations. They worked and they persisted after refreshing.
- I tested adding custom values in the fields and also updating the values after.
In regards the code I didn't see any blocker. I left some 💅 and some questions. But as they are not blockers I approve this in advance.
├── product-select-field/ # A custom block | ||
│ ├── block.json # The metadata file of this block and also the canonical way to register this block with both PHP and JavaScript side | ||
│ └── edit.js # The component to work with the `edit` function when registering this block, and it's the interface for how this block is going to be rendered within the Product Block Editor | ||
├── another-field/ # Another custom block | ||
│ ├── block.json | ||
│ └── edit.js |
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 documenting the structure. I'd suggest adding the Custom Blocks in their folder (ie custom_blocks, fields... ) so they are not at the same level as the components folder.
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.
Since at most there will only be 3 or 4 custom blocks, I lead toward not using a deeper directory structure.
@@ -130,6 +130,10 @@ public function get_uri(): string { | |||
* @return bool | |||
*/ | |||
public function can_enqueue(): bool { | |||
if ( is_null( $this->enqueue_condition_callback ) ) { |
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.
💅 Can we cover this by test unit as well?
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.
Added in e4a743e.
@@ -0,0 +1 @@ | |||
{} |
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.
❓ Why is this file for?
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.
For testing the registration of custom blocks without actually building client files.
google-listings-and-ads/tests/Unit/Admin/Product/Attributes/AttributesBlockTest.php
Lines 210 to 235 in 2eaee14
public function test_register_custom_blocks() { | |
$custom_blocks = [ 'existing-block', 'non-existent-block' ]; | |
$expected_asset = new AdminScriptWithBuiltDependenciesAsset( | |
'google-listings-and-ads-product-blocks', | |
'tests/data/blocks', | |
GLA_TESTS_DATA_DIR . '/blocks.asset.php', | |
new BuiltScriptDependencyArray( | |
[ | |
'dependencies' => [], | |
'version' => (string) filemtime( GLA_TESTS_DATA_DIR . '/blocks.js' ), | |
] | |
) | |
); | |
$this->assets_handler | |
->expects( $this->exactly( 1 ) ) | |
->method( 'register' ) | |
->with( $expected_asset ); | |
$this->assets_handler | |
->expects( $this->exactly( 1 ) ) | |
->method( 'enqueue' ) | |
->with( $expected_asset ); | |
$this->attributes_block->register_custom_blocks( GLA_TESTS_DATA_DIR, 'tests/data/blocks', $custom_blocks ); | |
} |
@@ -0,0 +1,35 @@ | |||
// Copied from WooCommerce core and simplified to leave only the needed parts. |
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 assume it will be done in future PRs. I expected to have some tests for the JS part as well
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.
Only using jest to test this component or custom blocks would be fragile as they involve a lot of Woo core mechanisms and codes. I planned to test them via E2E tests after all required development settled down.
"_templateBlockHideConditions": { | ||
"type": "array" | ||
}, |
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.
❓ Why is this for? When I delete it I don't see any difference at first look
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.
__experimentalRole
is an undocumented attribute property, and is commonly set in WooCommerce core, but it seems a bit paradoxical. Although I did find the use of its source code, it's not clear what it's used for, only knew that: without this property, it may lead to a custom block in editing-disabled mode.
As the product block editor in Woo core is still under development, this issue may be/has been resolved. However, it would be safer to keep it for the time being.
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 the explanation. I was referring to
"_templateBlockHideConditions": {
"type": "array"
},
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.
Sorry, I got it wrong.
In order to make a custom block compatible with the conditionally hiding feature, it requires the _templateBlockHideConditions
attribute when registering the custom block on both PHP and JS sides.
On JS side, a custom block can use registerProductEditorBlockType to inject that attribute, but on PHP side, there is no corresponding way to do so. Therefore, a custom block still needs to do a similar injection as BlockRegistry::register_block when calling Gutenberg register_block_type
.
The good news is Woo 8.5.0 will have the core support by PR 41973 - Template API: Registration of custom block types, so the _templateBlockHideConditions
attribute will be removed in a subsequent PR.
"tooltip": { | ||
"type": "string" | ||
}, | ||
"property": { |
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.
❓ Same for these "property" and "context". I don't see clear references out there but seems to be an essential part of the config.
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 property
value will be passed into the corresponding block and be used with the below useProductEntityProp
to access a property or meta of product data. The naming is to align with all generic product blocks in Woo core, e.g. number, checkbox, radio and so on.
google-listings-and-ads/js/src/blocks/product-select-field/edit.js
Lines 15 to 17 in 2eaee14
const [ value, setValue ] = useProductEntityProp( attributes.property, { | |
postType: context.postType, | |
} ); |
Not sure if the "context" refers to useContext
. If yes, I added its use by this commit in PR #2178
…t` via `ScriptAsset`. Address: #2164 (comment)
…ct-block-editor-custom-block-infra
…d test case." This reverts commit 2eaee14.
…ct-block-editor-custom-block-infra
cb432d9
into
feature/support-product-block-editor
Changes proposed in this Pull Request:
To be compatible with Product Block Editor, this PR adds fundamental support:
can_enqueue
ofBaseAsset
as it should allow$enqueue_condition_callback
to be null.AttributesBlock
to register the custom blocks and the related assets.@woocommerce/dependency-extraction-webpack-plugin
package to get the support of the newer WC packages.To show how to develop and use the custom product blocks, this PR:
Select
andBooleanSelect
inputs to the custom block of the select field.Screenshots:
📷 Simple product
📷 Variable product
📷 Variation product
Detailed test instructions:
📌 Prepare test environment
npm install
npm start
📌 Test the simple product type
📌 Test the variable and variation product type
📌 Test the related filters
"woocommerce_gla_product_attribute_value_options_{$attribute_id}"
Additional details:
With WooCommerce 8.3.0, the tooltip is not aligned well, and the generic product blocks don't support the
tooltip
attribute.These issues will be resolved in WooCommerce 8.4.0, so this PR won't take care of them.
Changelog entry