-
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
Show Ads value prop in Paid Campaign performance card #2650
Show Ads value prop in Paid Campaign performance card #2650
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2460-google-ads-value-prop #2650 +/- ##
====================================================================
+ Coverage 63.5% 63.6% +0.1%
====================================================================
Files 331 332 +1
Lines 5214 5210 -4
Branches 1273 1265 -8
====================================================================
+ Hits 3313 3315 +2
+ Misses 1723 1718 -5
+ Partials 178 177 -1
Flags with carried forward coverage won't be shown. Click here to find out 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.
Nice work! I've left a couple suggestions.
Additionally, I noticed that the heights of each card do not match the designs:
I think this will get the right effect:
diff --git a/js/src/dashboard/index.scss b/js/src/dashboard/index.scss
index c1dfa3b54..417e5efb8 100644
--- a/js/src/dashboard/index.scss
+++ b/js/src/dashboard/index.scss
@@ -26,6 +26,7 @@
margin-bottom: var(--main-gap);
@include break-medium {
+ align-items: flex-start;
flex-direction: row;
}
I also noticed that if you create a campaign and then delete it, the promo is no longer being shown. That's because the logic to display the promo relies on the adsSetupComplete
global in this line, which causes this to have the same limitations as the useAdsCampaign
hook that is being discussed in this conversation, so we may want to update this as well depending on how that conversation is resolved.
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.
This looks better to me. Thanks!
QA Report- ✅Testing Environment -
Test Results: Implementation works as expected. Google Ads content appears in the Google Ads section on the dashboard when the Ad account doesn’t have any paid campaigns. The "Create Campaign" button redirects to the campaign creation flow. If the user creates a paid campaign, this new content section does not appear. ✅ Functional Demo / Screencast - Gogole.Ads.card.mp4 |
@kt-12 @joemcgill During testing, I noticed that when a merchant creates a new paid campaign, the new section disappears from the Google Ads screen. However, if the merchant deletes the paid campaign and there are no active paid campaigns, this screen content doesn’t reappear. Is this the expected behaviour? |
@ankitguptaindia This is due to a known issue, and currently the expected behaviour, too. cc: @joemcgill |
Great question @ankitguptaindia! Sine the promotion is meant to encourage someone to create their first campaign, I believe this is the expected behavior. |
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 first Acceptance Criteria:
When the connected Ads account does not have any existing campaigns, display the Paid Ads features with a CTA to create a campaign.
Currently, the promotion is only displayed before completing the Ads Setup flow. Excluding the considerations mentioned in #2641 (comment) regarding the existence of a connected account and adsSetupComplete
, it still doesn't meet this requirement.
[...] However, if the merchant deletes the paid campaign and there are no active paid campaigns, this screen content doesn’t reappear. Is this the expected behaviour?
[...] This is due to a known issue, and currently the expected behaviour, too.
I would like to emphasize again that the "issue" recognized in that comment is imprecise, and referencing it as a "known issue" is questionable. It's a flag defined by this plugin for identifying if the merchant has completed the Ads Setup flow.
The existing logic using it for determination is because those requirements are based on that definition. If a requirement couldn't be satisfied by its definition, it should not be misplaced as a "known issue" as a reason for not being able to implement the requirement.
js/src/dashboard/summary-section/paid-features/free-ad-credit.js
Outdated
Show resolved
Hide resolved
js/src/dashboard/summary-section/paid-features/free-ad-credit.js
Outdated
Show resolved
Hide resolved
I agree here that this is definitely within the boundaries of what was defined. Calling it a "known issue" issue would be wrong; all I wanted to point to @ankitguptaindia was that this behaviour is |
adding fires info Co-authored-by: Eason <[email protected]>
fix context Co-authored-by: Eason <[email protected]>
…m/woocommerce/google-listings-and-ads into feature/2538-performance-card-sep
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.
This looks very close. Left a couple suggestions, otherwise, once the Jest tests are sorted, this is working as expected.
@eason9487 I believe everything is addressed here. I've made a few additional style tweaks to get closer to the Figma designs, but as you've noted, this was designed with a different icon set than what the plugin is currently using. I've tried to match as best as possible. |
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.
Hi @joemcgill
Thanks for the style adjustments!
I believe everything is addressed here.
The #2650 (comment) and #2650 (comment) are not resolved.
js/src/dashboard/summary-section/paid-features/free-ad-credit.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Eason <[email protected]>
Co-authored-by: Eason <[email protected]>
Co-authored-by: Eason <[email protected]>
Hi @eason9487, I have pushed e2e changes, which will address the remaining review comments. |
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 work. LGTM.
Changes proposed in this Pull Request:
Closes #2538.
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
In this task, we have also deprecated the
useFreeAdCredit()
check the last point in IB.Test clicking
Add Paid Campaign
,Spend $500 to get $500
should only be shown if there is no campaign.Additional details:
We have used the common FreeAdCredit component here, which standardises the text and various links associated with that component.
Changelog entry