-
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 a custom block for prompting user to complete onboarding #2237
Product Block Editor: Add a custom block for prompting user to complete onboarding #2237
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/support-product-block-editor #2237 +/- ##
======================================================================
Coverage 61.5% 61.5%
- Complexity 4184 4185 +1
======================================================================
Files 748 748
Lines 21529 21538 +9
Branches 532 532
======================================================================
+ Hits 13232 13244 +12
+ Misses 7846 7843 -3
Partials 451 451
Flags with carried forward coverage won't be shown. Click here to find out more.
|
b6ccd44
to
d75ee28
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 for the changes @eason9487 ✅ LGTM
- I tested the
product-onboarding-prompt
and I can see it when the onboarding is not completed. - When it's completed then it shows the GLA blocks instead.
❓Not introduced in this PR but it seems that ChannelVisibilityBlock::update_data
and ChannelVisibilityBlock::prepare_data
have the wrong type in the params. Shouldn't be just WC_Product
instead of WC_Data
?
Actually. We return if $product
is not WC_Product
if ( ! $product instanceof WC_Product || ! in_array( $product->get_type(), $this->get_visible_product_types(), true ) ) {
return;
}
if ( ! $this->merchant_center->is_setup_complete() ) { | ||
return; | ||
} |
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 not make this controller conditional 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.
The Conditional::is_needed
method is static but the MerchantCenterService::is_setup_complete
method is not static, so this service cannot be registered conditionally.
$this->variation_anchor_group->get_root_template() | ||
->expects( $this->exactly( 0 ) ) | ||
->expects( $this->exactly( 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.
💅 Another simple version for $this->exactly( 1 )
and $this->exactly( 0 )
that we normally use is $this->once()
and $this->never()
. Not against using $this->exactly()
just mentioning it in case you didn't know.
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 information. I chose exactly
because it's relatively effortless to maintain.
Hi @puntope, thanks for the review!
The code reviews in #2214 did discuss this, please refer to #2214 (comment) and #2214 (comment). |
…n as it has different conditions than `ProductBlocksService`.
… and register custom blocks when the onboarding is not yet completed.
…ocksService` class.
222d5e1
to
20d8e0c
Compare
c08f614
into
feature/support-product-block-editor
Changes proposed in this Pull Request:
With the original product editor, this extension shows a prompt if the user has not yet completed onboarding.
To have a consistent prompt in the Product Block Editor, this PR:
ChannelVisibilityBlock
handle its own registration as it has different conditions thanProductBlocksService
.ProductBlocksService
to still add a group for this extension and register custom blocks when the onboarding is not yet completed.woocommerce-grow-jsdoc
andwoocommerce-grow-storybook
.${var}
in a string for theProductBlocksService
class.Screenshots:
Kapture.2024-02-05.at.18.26.04.mp4
Detailed test instructions:
📌 Prepare test environment
npm install
npm start
google-listings-and-ads/src/MerchantCenter/MerchantCenterService.php
Lines 75 to 77 in 1ef2067
📌 Test the onboarding prompt
Changelog entry