-
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
Streamline campaign setup: Show campaign setup fields immediately during onboarding #2533
Streamline campaign setup: Show campaign setup fields immediately during onboarding #2533
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/2459-campaign-creation-flow #2533 +/- ##
===================================================================
Coverage 63.8% 63.8%
===================================================================
Files 326 326
Lines 5089 5089
Branches 1232 1232
===================================================================
Hits 3247 3247
Misses 1674 1674
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.
@asvinb this is looking pretty good. Left a few suggestions.
} ); | ||
} | ||
merge( eventProps, { | ||
opened_paid_ads_setup: 'yes', |
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.
Now that we're removing the possibility of skipping without opening the ads setup, this event prop should be removed from the whole component and removed from the inline docs for gla_onboarding_complete_button_click
above.
I think we could also avoid the need to merge event props here and just set them directly when eventProps
is initialized above.
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.
Good catch and makes sense @joemcgill . I've updated the PR. Can you kindly take a look again 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 updates. Sorry I missed that there were already separate tests for the second "Skip paid ads creation" button, your previous changes look correct.
QA/Test Report- ✅Testing Environment -
Steps to Test- Please follow the steps mentioned in PR description and watch below attached screencast for all screens. Test Results - Tested the changes in onboarding flow, and all scenarios are functioning as expected.
Functional Demo / Screencast - Recording.803.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.
Hi @asvinb, I tested this locally, and it’s working well. I just have a question about the event props—previously, the default value was unknown
for some of the properties, but now the behavior has changed. Let me know your thoughts. Thanks!
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 updates seem ok to me. @eason9487 can you give it another look?
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
@asvinb can you take a look at the merge conflicts here? |
…show-campaign-fields
…merce/google-listings-and-ads into feature/2500-show-campaign-fields
Tested again after the merge conflicts were adjusted. Merging. |
5ac539f
into
feature/2459-campaign-creation-flow
Changes proposed in this Pull Request:
Closes #2500 .
Screenshots:
Detailed test instructions:
Create a campaign
, the fields to create a campaign should be shown by default.Additional details:
Changelog entry