Closed joemcgill closed 4 days ago
Attention: Patch coverage is 50.00000%
with 1 line
in your changes missing coverage. Please review.
Project coverage is 63.6%. Comparing base (
2c3f089
) to head (10dc1c9
). Report is 10 commits behind head on feature/2460-google-ads-value-prop.
Files with missing lines | Patch % | Lines |
---|---|---|
...src/components/paid-ads/asset-group/asset-group.js | 0.0% | 1 Missing :warning: |
This is a quick one to update some UI text based on feedback from @fblascogarma during the review of #2460.
Thanks @mikkamp. I've addressed the two inline suggestions. For your other observations:
I noticed that when you click the edit campaign button the following popup is still shown which mentions a paid campaign:
Good catch. I'm not sure if we want to update the copy for these as well. I'll confirm with @fblascogarma.
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
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 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.
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.
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 EditProgramPromptModal
that is shown when editing both Free listings and Google Ads, the existing text was (emphasis mine) "Results typically improve with time with Google’s Free Listing and paid ad campaigns". I've chosen to replace this with "Results typically improve with time with Google’s Free Listing and Google Ads campaigns" to avoid implying that Ad campaigns are also free.
@fblascogarma feel free to review all the changes and suggest any additional tweaks.
Updated based on feedback in https://github.com/woocommerce/google-listings-and-ads/pull/2672#pullrequestreview-2438194580 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 https://github.com/woocommerce/google-listings-and-ads/pull/2670.
Changes proposed in this Pull Request:
Closes #2664.
Screenshots:
Detailed test instructions:
Additional details:
Changelog entry