-
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
Consolidate Ad Creation #2575
Consolidate Ad Creation #2575
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/2460-simplify-paid-ads-setup #2575 +/- ##
=========================================================================
- Coverage 65.0% 62.4% -2.7%
=========================================================================
Files 475 334 -141
Lines 17900 5226 -12674
Branches 0 1269 +1269
=========================================================================
- Hits 11640 3259 -8381
+ Misses 6260 1781 -4479
- Partials 0 186 +186
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 wanted to leave an early review with some observations I'm seeing. I can see that you have copied the onboarding flow's component, SetupPaidAds
, to a new AdsCampaign
component, which makes sense to me. However, in doing so, it doesn't include the changes made to that form related to #2459. I worry that this is going to lead to some challenging merge conflict resolution. Is it possible to try merging the feature/2459-campaign-creation-flow branch into this branch and ensure those changes are all included?
Additionally, to try to reduce some of the complexity juggling work in parallel with tickets like this one, #2502, and #2503, we should merge all of the PRs targeted to feature/2460-simplify-paid-ads-setup to the feature branch for #2459 and make them one feature rather than two.
/> | ||
) } | ||
|
||
<FaqsSection /> |
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 changes to the AdsCampaign component in #2531 need to be preserved here.
// because when there is no connected account, it will disable the budget section and set the `amount` to `undefined`. | ||
const disabledComplete = | ||
completing === ACTION_SKIP || ! paidAds.isReady || ! isValidForm; | ||
const shouldShowPaidAdsSetup = ! onboardingSetup || showPaidAdsSetup; |
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 shouldShowPaidAdsSetup logic was removed in #2533. Is there a reason it needs to be added back here?
@joemcgill The PR is no longer relevant unfortunately. I'll close it in favour of #2623 |
Changes proposed in this Pull Request:
Closes #2535
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
Additional details:
Changelog entry