-
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
Free Listings + Paid Ads: Add the Google Ads account setup section #1654
Free Listings + Paid Ads: Add the Google Ads account setup section #1654
Conversation
…ds account is disconnected
…management page of Google Ads account
…account from Google Ads account card in the onboarding flow
|
||
/** | ||
* @param {Object} props React props | ||
* @param {string} props.additionalScopeEmail | ||
* @fires gla_google_account_connect_button_click with `{ action: 'scope', context: 'setup-ads' }` | ||
*/ | ||
const AuthorizeAds = ( { additionalScopeEmail } ) => { | ||
const nextPageName = glaData.mcSetupComplete ? 'setup-ads' : 'setup-mc'; |
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.
💅 Maybe we can replace this string with constants like SETUP_ADS_PAGE and SETUP_MC_PAGE
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.
In this case, I think it's no need to declare additional constants. They are not duplicated in the same scope of uses. The 'setup-ads'
below this file belong to another semantic and use of context, except that they happen to be the same value.
In terms of the whole codebase, whether to create shared constants for event tracking properties such as the context
will be another 📜 topic.
// - Google Ads account connection | ||
// - Campaign data | ||
// - Billing setup | ||
const disabledComplete = completing === 'skip-ads'; |
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.
💅 Id use this skip-ads as a constant SKIP_ADS
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.
As per the suggestion in #1649 (comment), I have added constants in 0e65308 and c08364f.
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
- I tested normal workflow and Iw as able to create new Ads account, selected existing account and switch accounts
- I tested by removing permissions for Ad management and I saw the Allow access button which is working
- UI match figma
💅 Left some minor comments in code
💅 Missing tests
Changes proposed in this Pull Request:
This PR implements the 📌 Google Ads account section in #1610.
AD_WORDS
authorization of Google account from Google Ads account card in the onboarding flow.This PR also fixes two issues:
:active
CSS style for disabled AppButtonScreenshots:
Kapture.2022-08-26.at.17.17.23.mp4
Detailed test instructions:
💡 When re-entering step 4, the previously selected "continue setting up paid ads" state is reset. Keeping the selected state will be developed in another PR.
Changelog entry