-
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 support and map Text
and Integer
attribute inputs to currently available generic blocks
#2151
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/support-product-block-editor #2151 +/- ##
========================================================================
+ Coverage 58.2% 60.6% +2.4%
- Complexity 4127 4144 +17
========================================================================
Files 453 453
Lines 17691 17785 +94
========================================================================
+ Hits 10300 10777 +477
+ Misses 7391 7008 -383
Flags with carried forward coverage won't be shown. Click here to find out more.
|
c91d7f0
to
406382b
Compare
75e1ace
to
9a81d84
Compare
Text
and Number
attribute inputs to currently available generic blocksText
and Integer
attribute inputs to currently available generic blocks
…` to `AttributesTrait`.
…s in `AttributesForm` as a method.
… Editor for product attributes.
…heritance classes.
…ct\Attributes\Input`.
9a81d84
to
0408038
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.
Thanks @eason9487 for working on this, I ran some tests with simple products, and things went smoothly, except for the issue I mentioned in the comment about encountering an error with the filter.
Regarding the variation you mentioned:
Within the Google Listings & Ads section, there should be only a brand field for now. Operate the brand field and change its value.
All I'm seeing is a blank section.
The filters didn't seem to affect anything when I tested them with the variations.
I ran out of time today, but I am happy to debug this issue deeper if you are not able to replicate it.
* @param BlockInterface $reference_section The reference section block to add this extension's section block | ||
*/ | ||
private function add_section( BlockInterface $reference_section ): BlockInterface { | ||
$group = $reference_section->get_parent(); |
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.
Would be nice to have the type reference here: /** @var Automattic\WooCommerce\Internal\Admin\Features\ProductBlockEditor\ProductTemplates\Group */
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.
* | ||
* @param BlockInterface $reference_section The reference section block to add this extension's section block | ||
*/ | ||
private function add_section( BlockInterface $reference_section ): BlockInterface { |
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.
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! This method will be removed by this commit in PR #2179. To avoid future code conflicts, I would skip this adjustment.
return; | ||
} | ||
|
||
$section = $this->add_section( $attributes_section ); |
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.
Initially this part confused me, as it seems that the method add_section
would add the $attributes_section
. However, it turns out the 'add' method retrieves the group from this attribute section and subsequently adds a new section. I'm thinking, perhaps renaming this function to something like 'add_section_to_group' could better clarify its purpose.
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 suggestion.
This method call will be inlined by this commit in PR #2179. Instead of using $this
to indirectly add group and section to template, the changed code fragment now calls to the literal callee, which I believe should not cause similar confusion as here.
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, yes that will remove the ambiguity!
} | ||
|
||
$section = $this->add_section( $attributes_section ); | ||
$this->add_blocks( $section ); |
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.
Similar to my earlier comment, this could be add_blocks_to_section
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.
In this commit of PR #2179, this line will be changed to $this->add_product_attribute_blocks( $product_attributes_section );
, which should be able to clarify the purpose that it adds product attribute blocks to a section.
add_action( | ||
"woocommerce_block_template_area_{$template_area}_after_add_block_{$block_id}", | ||
function ( BlockInterface $attributes_section ) { | ||
// Please note that the simple and variable product types use the same product block template 'simple-product'. |
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.
Do we want to keep the type generic using BlockInterface
? Or can we use Automattic\WooCommerce\Internal\Admin\Features\ProductBlockEditor\ProductTemplates\Section
?
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.
In the definition of this hook, there are several possible incoming instances, and each of them is a class that implements BlockInterface
.
Although in runtime it passes in a Section
instance, I think it also makes sense to use the more generic type BlockInterface
here. Like when I adjusted their insertion position in #2179, the type reference here didn't need to be changed.
/** | ||
* Get the hidden and visible types of an attribute's applicable product types. | ||
* | ||
* @param string $attribute_type An attribute class extending AttributeInterface |
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.
Maybe An attribute classname ....
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 don't understand this comment. Could you explain a bit more?
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.
My bad, I should have clarified this better 😅. What I meant was in the PHP doc, when we mention 'An attribute class,' strictly speaking, it refers to the actual name of that class. So, my suggestion was more along the lines of asking whether we should update this to: 'The name of the Attribute class extending AttributeInterface.'
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 suggestion! Updated all related PHP docs for the AttributesForm
class by 9ff6a97.
/** | ||
* Filters the list of product types to hide the attribute for. | ||
*/ | ||
$hidden_product_types = apply_filters( "woocommerce_gla_attribute_hidden_product_types_{$attribute_id}", [] ); |
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.
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.
…mpty string that causes errors. Address: #2151 (comment)
Hi @jorgemd24, thanks for the review.
Could you check if WooCommerce Brands is activated? If it is, the google-listings-and-ads/src/Admin/Product/Attributes/AttributesBlock.php Lines 130 to 133 in 46830dc
If this is not the cause, then I can't reproduce it, so I may need your help to find the cause. |
…ot visible. Address: #2151 (comment)
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 @eason9487, for fixing the issue and your comments. The reason why the brands field is not displayed was because the SelectInput doesn't have a block name, therefore the code is skipping this field. For this test, I added the blockname 'woocommerce/product-text-field'
and I could test the behaviour. I see that you already work on this here https://github.com/woocommerce/google-listings-and-ads/pull/2178/files#diff-eb72915300551a54c7e5237203c3878b9c0bcedb253bb0d0624e45890e7b so I am approving this PR as everything works nos smoothly.
Thanks again!
…eters in the `AttributesForm` class. Address: #2151 (comment)
e1798ea
into
feature/support-product-block-editor
Changes proposed in this Pull Request:
To be compatible with Product Block Editor, this PR adds fundamental support:
AttributesForm
as a method.init_input
method inAttributesForm
to a static method.Admin\Input\Input
for getting block config.AttributesBlock
to register blocks to Product Block Editor for product attributes.Also, it turns a few attribute inputs to support Product Block Editor first:
Text
andInteger
inputs to currently available generic blocks.Last, it adds some PHP tests and removes a few unused PHP classes or method:
is_custom_value
fromSelectWithTextInput
.Admin\Input\Input
class and its inheritance classes.Admin\Product\Attributes\Input
.AttributesForm
class.AttributesBlock
class.Screenshots:
📷 Simple product
📷 Variation product
📷 The
min
attribute of the multipack inputDetailed test instructions:
📌 Prepare test environment
📌 Test the simple product type
📌 Test the variable and variation product type
📌 Test the related filters
"woocommerce_gla_attribute_applicable_product_types_{$attribute_id}"
"woocommerce_gla_attribute_hidden_product_types_{$attribute_id}"
Additional details:
The generic number block (
'woocommerce/product-number-field'
) doesn't support verifying/limiting for entering an integer. If it's not supported afterward, then we may have to convert it to a custom block.Changelog entry