Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Remove the billing setup page from the Ads setup flow #2577
base: feature/2459-campaign-creation-flow
Are you sure you want to change the base?
Remove the billing setup page from the Ads setup flow #2577
Changes from 22 commits
40ea4eb
8a9a10a
94c8208
39a2090
09871a5
5b14c07
9728b03
f06b5d2
3409919
0c74f98
779eac3
ef31827
e7afe1e
daa483a
71c81ec
b5a2467
0ca45f1
98cd72d
52ca19d
b8178b4
c5be7e0
1833363
d053b77
c8ee77f
1b31ca9
e7da33a
a6d79a0
7edeb3b
4ff5165
b90f77c
0c6d250
fdd3aae
104b368
2c84851
fe7bab6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Considering this button now requires 5 props to render and its only internal dependency is on one of those conditions, its degree of customization makes it unsuitable to be part of this component. It's better to externalize it as a single prop such as
continueButton
to composite this component.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.
@eason9487 I believe this continue button is only temporarily going to need to handle this many conditions until #2535 is completed, to ensure that the current flow could still be tested properly. I'd like to avoid making any additional modifications to the
AdsCampaign
component in this PR, which is really only meant to remove the billing step because it will be handled during step 3 once we've consolidated the form from the onboarding flow.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.
google-listings-and-ads/js/src/components/paid-ads/ads-campaign/ads-campaign.js
Lines 103 to 117 in 34c53d7
google-listings-and-ads/js/src/components/paid-ads/ads-campaign/ads-campaign.js
Lines 176 to 182 in 34c53d7
From the current revision of
AdsCampaign
in #2623, I couldn't figure out how this button only temporarily needs to handle 5 props, because both PRs currently increase the complexity and conditional branching of this button to varying degrees.All of these changes convinced me that both PRs should try to see if it's possible to move this button out of the
AdsCampaign
component sooner rather than having to resolve more code conflicts and re-test all button scenarios when merging them with each other.Check failure on line 90 in js/src/setup-ads/ads-stepper/index.test.js
GitHub Actions / JavaScript unit tests
Check failure on line 90 in js/src/setup-ads/ads-stepper/index.test.js
GitHub Actions / JavaScript unit tests
This file was deleted.
This file was deleted.
This file was deleted.