-
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
Improve post-onboarding: Remove the word "Paid" from the plugin #2672
Improve post-onboarding: Remove the word "Paid" from the plugin #2672
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2460-google-ads-value-prop #2672 +/- ##
==================================================================
Coverage 63.6% 63.6%
==================================================================
Files 332 332
Lines 5210 5210
Branches 1265 1265
==================================================================
Hits 3315 3315
Misses 1718 1718
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This is a quick one to update some UI text based on feedback from @fblascogarma during the review of #2460. |
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!
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 these changes, I can confirm that those screens now show the updated text.
I noticed that when you click the edit campaign button the following popup is still shown which mentions a paid campaign:
I was also a bit confused about going through the onboarding and creating a campaign. I was expecting to be redirected to the dashboard at the following URL: /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&guide=campaign-creation-success
But instead I was redirected to the product feed tab with the following modal: /wp-admin/admin.php?page=wc-admin&guide=submission-success&path=%2Fgoogle%2Fproduct-feed
Is there another PR where that behaviour was changed intentionally?
For the rest everything looks pretty good, just a few small minor comments about renaming some additional test lines.
I wasn't able to confirm the E2E tests are still passing as it fails with a missing currency in the text. I don't see that being directly related to this PR, but possibly some of the dependent code changed this.
1) [chromium] › setup-mc/step-4-complete-campaign.test.js:259:5 › Complete your campaign › Set up paid ads › Set up a campaign › Validate budget percent › should see validation error if lower than the 30%
Error: Timed out 20000ms waiting for expect(locator).toHaveText(expected)
Locator: locator('.components-base-control__help')
Expected string: "Please make sure daily average cost is at least NT$30.00"
Received string: "Please make sure daily average cost is at least 30.00"
@@ -280,13 +280,13 @@ test.describe( 'Set up Ads account', () => { | |||
} ); | |||
} ); | |||
|
|||
test.describe( 'Create your paid campaign', () => { | |||
test.describe( 'Create your campaign', () => { | |||
test( 'Continue to create paid ad campaign', async () => { |
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 other test descriptions are being updated, we can change this one 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.
Addressed in 4ac7c89
Co-authored-by: Mik <mikkamp@users.noreply.github.com>
Thanks @mikkamp. I've addressed the two inline suggestions. For your other observations:
Good catch. I'm not sure if we want to update the copy for these as well. I'll confirm with @fblascogarma.
The onboarding flow has ended with the user redirected to the Product Feed tab since at least this commit, so I assume that has been an intentional decision.
I'm unable to reproduce this E2E test failure. Just to be safe, I triggered another full test run here, which seems to be passing. |
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 are other places that may not be within the campaign creation scope, but have the word "paid", which could be further confirmed.
Get Started
Dashboard
Edit Campaign
- Can be found with path
/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&subpath=%2Fcampaigns%2Fedit&programId=0
Reports
Reports > Products
Settings
Inbox notifications
Good catch @eason9487 ! Yes, please let's remove it from those places too. Thank you!! |
The additional instances of 'paid' have been replaced in the last two commits. In each case, I've tried to use my best judgement in when it was appropriate to replace the existing text with "Google Ads", "campaign(s)", based on the context in which they are found. For example, in the @fblascogarma feel free to review all the changes and suggest any additional tweaks. |
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 are still a few left over but it shouldn't be a need for another round.
'Skip paid ads creation', |
google-listings-and-ads/tests/e2e/utils/pages/setup-mc/step-4-complete-campaign.js
Line 77 in cef1b83
name: 'Skip paid ads creation', |
src/Notes/SetupCampaignTwoWeeks.php
Outdated
@@ -48,7 +48,7 @@ protected function set_title_and_content( NoteEntry $note ): void { | |||
$note->set_title( __( 'Reach more shoppers with paid listings on Google', 'google-listings-and-ads' ) ); |
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 title has the word "paid" as well.
Updated based on feedback in #2672 (review) and this edit from @fblascogarma. Cherry-picked a70da30 in order to E2E Tests and it looks like everything is passing, except for a known issue fixed in #2670. |
98c62a5
into
feature/2460-google-ads-value-prop
Changes proposed in this Pull Request:
Closes #2664.
Screenshots:
Detailed test instructions:
Additional details:
Changelog entry