-
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
2501: Remove Audience section from paids ads setup. #2551
2501: Remove Audience section from paids ads setup. #2551
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2459-campaign-creation-flow #2551 +/- ##
===================================================================
Coverage 63.8% 63.8%
===================================================================
Files 326 326
Lines 5089 5089
Branches 1231 1231
===================================================================
Hits 3245 3245
Misses 1676 1676
Partials 168 168
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.
This looks like it's working properly. Left feedback on several e2e test utils that I think can be removed since they were only being used in tests that you've removed in this PR.
Thanks @joemcgill . PR updated! |
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!
QA/Test Report- ✅Testing Environment -
Steps to Test- Please follow the steps mentioned in the PR description and watch below-attached screencast. Test Results - Tested the changes in onboarding flow, and all scenarios are functioning as expected.
Functional Demo / Screencast - Recording.804.mp4Next Step- Ready to Code Review(Woo) Plugin file with this change-related code: |
@eason9487 this one is ready for your review |
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 on this, @asvinb, it’s working well. I left a small comment regarding the default parameter for targetAudience
. Let me know what you think. Thanks!
} | ||
} | ||
|
||
function resolveInitialPaidAds( paidAds, targetAudience = [] ) { |
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.
By setting an empty array as the default parameter, it seems targetAudience isn’t required, and we can create ads without specifying any countries.
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 @jorgemd24 . Happy to revert that change but when I tested the "Continue" button to proceed to the next step was disabled if there were no countries selected. Screenshot below:
Also, in the validateCampaign
function, there's a check for an empty array of countries.
if ( values.countryCodes.length === 0 ) { |
Let me know what you think 😁
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!
Since the audience field no longer interacts with the form and has no need to be stored/restored via sessionStorage
, I would suggest further adjustments to make the code more consistent with its purpose:
- Decouple the
countryCodes
value fromclientSession
- Decouple the
countryCodes
value and relevant logic from<Form>
- Remove it from
initialValues
prop - Remove it from
validate
prop - Remove
disabledAudience
anddisabledBudget
variables because they are not applicable anymore
- Remove it from
- Update the related code comment
google-listings-and-ads/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js
Line 55 in 50b13df
* Renders sections of Google Ads account, audience, budget, and billing for setting up the paid ads.
In addition, the description in the budget field looks a bit confusion to users after removing audience field. Perhaps some adjustments could be made.
js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js
Outdated
Show resolved
Hide resolved
@joemcgill Can you kindly take a look at the updated PR please? |
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.
These changes all seem to be working. My only concern about decoupling all of the form data and removing the validation is whether those will be needed during #2535 once we're using the same component for creating ads during onboarding and from the dashboard after onboarding. Currently, with those forms being totally separate, the one responsible for creating Ads from the dashboard does not include the validation logic at all and still works with these changes, so it's probably fine to remove in this PR.
@ankitguptaindia could you give this another looks and check that this doesn't cause a regression in the flows used when creating or editing existing ads from the dashboard?
I checked the scenarios below and found them working:
Configured ads multiple times and couldn't face any trouble in all cases. |
@eason9487 Can you kindly take another look at the PR please? |
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 improvement!
There is a potential issue that needs to be resolved.
js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js
Outdated
Show resolved
Hide 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.
Thanks for the further adjustment! LGTM.
…remove-ads-audience
Changes proposed in this Pull Request:
Closes #2501
AudienceSection
from thePaidAdsSetupSections
component.countryCodes
prop in thepaidAds
state variable is now initialized with the values oftargetAudience
which itself contains the values from the second step of the onboarding process.Detailed test instructions:
Additional details:
Changelog entry