Setup Paid Ads Campaign Step 1 Account Connection #301

ecgan · 2021-03-08T15:25:04Z

Changes proposed in this Pull Request:

Closes #167 .

This PR provides the Step 1 in Setup Paid Ads Campaign. You will be able to create new Google Ads account or link an existing Google Ads account.

Screenshots:

Detailed test instructions:

Open https://gla1.loca.lt/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsetup-ads
Proceed with connecting Google account and Google Ads account. The end result should resemble the screenshot above.

This is so that it can be reused in Setup Ads flow.

Conflicts: js/src/setup-ads/ads-stepper/setup-accounts/index.js

js/src/setup-ads/ads-stepper/setup-accounts/google-ads-account-section/terms-modal/index.js

js/src/setup-ads/ads-stepper/setup-accounts/google-ads-account-section/create-account-button.js

js/src/components/account-select-control/index.js

js/src/data/reducer.js

eason9487 · 2021-03-09T08:53:01Z

js/src/setup-ads/ads-stepper/index.scss

+.gla-setup-stepper {
+	.woocommerce-stepper__steps {
+		align-items: center;
+		justify-content: center;
+		height: 64px;
+		background-color: #fff;
+		box-shadow: inset 0 -1px 0 #ccc;
+		margin-bottom: 0;
+
+		.woocommerce-stepper__step-divider {
+			margin-top: 0;
+			align-self: auto;
+			max-width: 48px;
+		}
+	}
+}


Here are duplicated styling to setup-mc/setup-stepper/index.scss

Yeah I'm aware of that 😄 👍 which is why I added a comment in js/src/setup-ads/ads-stepper/index.js that we should refactor it some time later after this PR gets merged. I think @tomalec is / was also making changes to the setup stepper and we can de-dupe it later when everything is stable.

tomalec · 2021-03-09T12:07:07Z

I'm not sure whether it's something to report here or in a separate issue, but when I was testing this page with a Google account, linked with MC account, but without any Ads account, I was getting quite an enigmatic result:

{"message":"Error retrieving accounts: Request is missing required authentication credential. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https:\/\/developers.google.com\/identity\/sign-in\/web\/devconsole-project."}

There was a toast that disappeared with: There was an error getting your Google Ads accounts.

The problem was solved once I go to https://ads.google.com/intl/pl_pl/getstarted/? and created an Ads account associated with my test user G account. However, if I was a new-coming merchant I doubt I would guess what to do.

mikkamp · 2021-03-09T12:13:01Z

I'm not sure whether it's something to report here or in a separate issue, but when I was testing this page with a Google account, linked with MC account, but without any Ads account, I was getting quite an enigmatic result

That's been tracked here: #291

js/src/setup-ads/index.js

tomalec · 2021-03-09T11:36:07Z

js/src/dashboard/all-programs-table-card/index.js

-						Add Paid Campaign
-					</Button>
+						{ __( 'Add Paid Campaign', 'google-listings-and-ads' ) }
+					</Link>


Probably it was not introduced here, but the placement of this link seems off from Figma:

Figma:

Actual:

BTW, "All programs" seem to be just "Programs" now, but this is a good candidate for a separate PR

It looks fine for me, accessed from https://gla1.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard (I changed to "Programs", thanks for the heads up):

Strange, for me it looks different, and according to what I see in .woocommerce-table .components-card__header { /*...*/ grid-template-columns: auto 1fr auto;
, it should look that way

Anyway, it does look the same on trunk, so it's not the matter of this PR.

@tomalec , thanks for sharing your screenshot. That is weird. Here's my screenshot, based on current trunk branch:

The thing is, within my "app-table-card" div, I have woocommerce-card but you have components-card. I look into the code in js/src/components/app-table-card/index.js, it should render WooCommerce Card. Do you have any local changes in your machine?

I have the same results as Tomek. 👀

I found some clues but have no idea how to fix that in the right way. When the WooCommerce Admin is activated in the Plugins page, it would load v1.9.0 of @woocommerce/components. Otherwise, it would load v1.8.3. And you can see they import different <Card> component sources. 😂

v1.9.0

https://github.com/woocommerce/woocommerce-admin/blob/v1.9.0/packages/components/src/table/index.js

import { Card, // ...emit } from '@wordpress/components';

v1.8.3

https://github.com/woocommerce/woocommerce-admin/blob/v1.8.3/packages/components/src/table/index.js

import Card from '../card';

@ecgan I believe once you activate the WooCommerce Admin plugin, you will see the same results. 😆

More precisely, the exactly loading version of @woocommerce/components should depend on the dependency of installed versions of the WooCommerce and WooCommerce Admin plugins.

@eason9487 , thanks so much, that's awesome troubleshooting! 😃 👍

@ecgan I believe once you activate the WooCommerce Admin plugin, you will see the same results. 😆

I do have WC Admin activated, but it is version 1.8.0 😂

I created an issue to track this: #320

js/src/setup-ads/ads-stepper/setup-accounts/index.js

Co-authored-by: Tomek Wytrębowicz <[email protected]>

jconroy · 2021-03-10T08:35:21Z

Nice @ecgan I was able to run through and connect an existing account 👍

One thing I noticed is I see an error message flash up briefly on the initial load

tomalec

Apart from flashing error, tests and looks good to me.
https://github.com/woocommerce/google-listings-and-ads/pull/301/files#r590264569 seems like a candidate for another PR.

Conflicts: js/src/components/stepper/top-bar/index.js

ecgan · 2021-03-10T14:11:25Z

I created a separate issue to track the flashing error here: #318

ecgan added 16 commits March 8, 2021 22:09

Add link to setup paid ads campaign page.

9093085

Add TopBar for Setup Ads flow.

cb5c142

Code refactor TopBar component to accept title props.

5aa1ede

This is so that it can be reused in Setup Ads flow.

Use TopBar component in SetupAds.

d9683a8

Add AdsStepper.

a080f21

Merge branch 'trunk' into feature/setup-ads-step-1

bd7b1d5

Display GoogleAccount in AdsStepper.

e893db7

Use p element for better layout spacing.

89e987a

Display GoogleAdsAccountSection.

113c0fd

Conflicts: js/src/setup-ads/ads-stepper/setup-accounts/index.js

Add audience section in CreateCampaign.

e6765c1

Code refactor with AccountSelectControl.

a11d10e

Add wp-data for Google Ads account.

aab8275

Add hooks for Google Ads account.

46c53c0

Move AccountId to shared components folder.

83afb7a

Move SpinnerCard to shared components folder.

9a516b3

Create new Google Ads account or link an existing account.

ad75a8e

ecgan requested a review from a team March 8, 2021 15:25

ecgan self-assigned this Mar 8, 2021

ecgan mentioned this pull request Mar 8, 2021

[DRAFT] Setup Paid Ads Campaign Step 1 Account Connection #293

Closed

tomalec mentioned this pull request Mar 8, 2021

Fork ChooseAudience component and use it in EditFreeCampaign #303

Merged

jconroy mentioned this pull request Mar 9, 2021

Discovery: Promote paid listings after free listings are configured #7

Closed

5 tasks

eason9487 reviewed Mar 9, 2021

View reviewed changes

tomalec requested changes Mar 9, 2021

View reviewed changes

ecgan and others added 5 commits March 9, 2021 22:48

Use import alias.

dfabf69

Co-authored-by: Tomek Wytrębowicz <[email protected]>

Fix termslink typo.

975b1ae

Better handler name to avoid confusion with the onCreateAccount props.

dd8ca71

Display "Account " text in AccountSelectControl label.

1e33bf3

Fix existing_ads missing in reducer default state.

de77b1d

Change "All Programs" label to "Programs" based on new design.

14d69e2

ecgan requested review from tomalec and eason9487 March 9, 2021 15:12

tomalec reviewed Mar 10, 2021

View reviewed changes

Merge branch 'trunk' into feature/setup-ads-step-1

36191e4

Conflicts: js/src/components/stepper/top-bar/index.js

ecgan mentioned this pull request Mar 10, 2021

Setup Ads Step 1 has error message flash up briefly on the initial load #318

Closed

ecgan merged commit a1d7404 into trunk Mar 10, 2021

ecgan deleted the feature/setup-ads-step-1 branch March 10, 2021 14:10

ecgan mentioned this pull request Mar 10, 2021

Programs table card layout issue #320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup Paid Ads Campaign Step 1 Account Connection #301

Setup Paid Ads Campaign Step 1 Account Connection #301

ecgan commented Mar 8, 2021

eason9487 Mar 9, 2021

ecgan Mar 9, 2021

tomalec commented Mar 9, 2021

mikkamp commented Mar 9, 2021

tomalec Mar 9, 2021 •

edited

Loading

ecgan Mar 9, 2021

tomalec Mar 10, 2021

ecgan Mar 10, 2021

eason9487 Mar 10, 2021

eason9487 Mar 10, 2021 •

edited

Loading

eason9487 Mar 10, 2021

ecgan Mar 10, 2021

jconroy commented Mar 10, 2021 •

edited

Loading

tomalec left a comment

ecgan commented Mar 10, 2021

Setup Paid Ads Campaign Step 1 Account Connection #301

Setup Paid Ads Campaign Step 1 Account Connection #301

Conversation

ecgan commented Mar 8, 2021

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented Mar 9, 2021

mikkamp commented Mar 9, 2021

tomalec Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eason9487 Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

v1.9.0

v1.8.3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jconroy commented Mar 10, 2021 • edited Loading

tomalec left a comment

Choose a reason for hiding this comment

ecgan commented Mar 10, 2021

tomalec Mar 9, 2021 •

edited

Loading

eason9487 Mar 10, 2021 •

edited

Loading

jconroy commented Mar 10, 2021 •

edited

Loading