Onboarding: Connect to existing Ads account in Google Accounts Card

[ankitrox]

Changes proposed in this Pull Request:

Closes #2596 .

Replace this with a good description of your changes & reasoning.

Screenshots:

Detailed test instructions:

There are mainly two scenarios to test for this issue.

Scenario 1

Both Google Ads and MC accounts are present for the Google account.

Scenario 2

Google Ads exists, but no MC account is associated with the Google account.

Install tweak extension in chromw browser to test both the scenarios.

Rules for scenario 1

Rule 1:

URL: https://domain-name.ngrok.app/index.php?rest_route=%2Fwc%2Fgla%2Fmc%2Fconnection&_locale=user
Method: GET
Delay: 2000ms
Response:

{
  "id": 1234567,
  "currency": "USD",
  "symbol": "$",
  "status": "connected",
  "step": "",
}

Rule 2:

URL: https://domain-name.ngrok.app/index.php?rest_route=%2Fwc%2Fgla%2Fads%2Faccounts&_locale=user
Method: GET
Delay: 2000ms
Response:

[
  {
    "id": 1234567,
    "subaccount": true,
    "name": "google-listings-and-ads",
    "domain": "https://domain-name.ngrok.app"
  }
]

Rules for scenario 2

Rule 1:

URL: https://domain-name.ngrok.app/index.php?rest_route=%2Fwc%2Fgla%2Fmc%2Fconnection&_locale=user
Method: GET
Delay: 2000ms
Response:

{
  "id": 0,
  "currency": "USD",
  "symbol": "$",
  "status": "disconnected",
  "step": "",
}

Rule 2:

URL: https://domain-name.ngrok.app/index.php?rest_route=%2Fwc%2Fgla%2Fads%2Faccounts&_locale=user
Method: GET
Delay: 2000ms
Response:

[]

Steps to test

1. Only enable the scenario related rules in extension. Enable extension by clicking play button.

2. Go to Step 1 of the onboarding, make sure to connect with Google account which already have existing ads accounts associated.

3. Once Google account is connected, you should see Connect to existing Google Ads account card with the ads accounts in dropdown.

4. If there is only one account, the dropdown won't be able to expand.

5. Clicking the connect button will trigger connection process and will connect to that ads account. You should be able to see ads account ID in card description once connected successfully.

6. For scenario 2, you should be able to see that merchant account is being created. It will always be in creation status because we are mocking that state via extension.

7. Once the ads account is connected, the dropdown won't be able to expand. It will show the Connected label beside dropdown.

8. If ads account is connected, text in footer should be Or, connect to a different Google Ads account.

Additional details:

Changelog entry

[codecov]

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.7%. Comparing base (7ceffde) to head (4cba145).
Report is 58 commits behind head on feature/2509-consolidate-google-account-cards.

Files with missing lines	Patch %	Lines
js/src/components/account-card/index.js	77.8%	2 Missing ⚠️

Additional details and impacted files

@@                              Coverage Diff                              @@
##           feature/2509-consolidate-google-account-cards   #2640   +/-   ##
=============================================================================
  Coverage                                           62.7%   62.7%           
=============================================================================
  Files                                                324     325    +1     
  Lines                                               5153    5162    +9     
  Branches                                            1259    1265    +6     
=============================================================================
+ Hits                                                3232    3239    +7     
- Misses                                              1744    1746    +2     
  Partials                                             177     177           

Flag	Coverage Δ
js-unit-tests	62.7% <87.5%> (+<0.1%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
js/src/components/app-select-control/index.js	88.9% <100.0%> (+0.7%)	⬆️
...gle-ads-account-card/connect-ads/connect-button.js	100.0% <100.0%> (ø)
...nents/google-ads-account-card/connect-ads/index.js	95.8% <100.0%> (-0.2%)	⬇️
js/src/components/account-card/index.js	91.9% <77.8%> (-4.9%)	⬇️

[asvinb]

Thanks @ankitrox . I left a few suggestions. Can you kindly check them out please?

[asvinb]

Just FYI, this will updated in https://github.com/woocommerce/google-listings-and-ads/pull/2639/files#diff-8c885f578f99827b77511ccb568a741365e07f9fd3eecea8bc62ac67253ac3a3R126

[asvinb]

@ankitrox I feel isConnected along is enough to display the ConnectedIconLabel component. Any idea why you want to check for the id as well?

[ankitrox]

@asvinb It checks whether:

1. Ads account is connected.
2. The currently selected ads account is the same which is connected.

In this case, we show Connected label instead of Connect button.

[asvinb]

@ankitrox Not sure if am following here. When in connected state, the select is disabled, meaning the only way to change the Ads ID is by disconnecting the Ads account.
https://www.figma.com/design/fqR0EHi63lWahRcVTKCcba/Google-Listings-%26-Ads-v2.x?node-id=7662-6154&node-type=frame&t=FZW0ZlwmMV6RKllP-0

So if we just check for isConnected is fine.

[eason9487]

There are some issues that need to be addressed or confirmed.

I didn't view the code changes related to e2e testing in detail since they were continuously pushed commits while I was reviewing.

---

About ConnectAccountCard component

Beyond that, I would like to propose modifying the AccountCard component so that it supports the layout required by GoogleComboAccountCard, in place of the ConnectAccountCard component in this PR and the respective customized implementations in the related PRs.

Scope to be supported by AccountCard

The target is to support these needs

1. It renders additional content and bottom links as well as enabling the indicator to align to additional content.
   
2. It enables the layout of additional content to be extended to the right edge.
   
3. The additional content it renders can also be used for this purpose, but only for layouts. The part inside the bordered area is a separate implementation, but it might be possible to try to reuse a similar implementation in AccountCard via the relevant PR.
   

The following git diff result shows how the AccountCard could be modified.

diff --git a/js/src/components/account-card/index.js b/js/src/components/account-card/index.js
index bebf7df10..a55370b42 100644
--- a/js/src/components/account-card/index.js
+++ b/js/src/components/account-card/index.js
@@ -125,6 +125,11 @@ const alignStyleName = {
 	top: `gla-account-card__styled--align-top`,
 };
 
+const indicatorAlignStyleName = {
+	...alignStyleName,
+	toDetail: 'gla-account-card__indicator--align-to-detail',
+};
+
 /**
  * Renders a Card component with account info and status.
  *
@@ -156,12 +161,16 @@ export default function AccountCard( {
 	alignIcon = 'center',
 	indicator,
 	alignIndicator = 'center',
+	detail,
+	expandedDetail = false,
+	actions,
 	children,
 	...restProps
 } ) {
 	const cardClassName = classnames(
 		'gla-account-card',
 		disabled ? 'gla-account-card--is-disabled' : false,
+		expandedDetail ? 'gla-account-card--is-expanded-detail' : false,
 		className
 	);
 
@@ -172,19 +181,15 @@ export default function AccountCard( {
 
 	const indicatorClassName = classnames(
 		'gla-account-card__indicator',
-		alignStyleName[ alignIndicator ]
+		indicatorAlignStyleName[ alignIndicator ]
 	);
 
 	return (
 		<Section.Card className={ cardClassName } { ...restProps }>
 			<Section.Card.Body>
-				<Flex gap={ 4 }>
-					{ icon && (
-						<FlexItem className={ iconClassName }>
-							{ icon }
-						</FlexItem>
-					) }
-					<FlexBlock>
+				<div className="gla-account-card__body-layout">
+					{ icon && <div className={ iconClassName }>{ icon }</div> }
+					<div className="gla-account-card__subject">
 						{ title && (
 							<Subsection.Title className="gla-account-card__title">
 								{ title }
@@ -200,13 +205,23 @@ export default function AccountCard( {
 								{ helper }
 							</div>
 						) }
-					</FlexBlock>
+					</div>
+					{ detail && (
+						<div className="gla-account-card__detail">
+							{ detail }
+						</div>
+					) }
 					{ indicator && (
-						<FlexItem className={ indicatorClassName }>
+						<div className={ indicatorClassName }>
 							{ indicator }
-						</FlexItem>
+						</div>
+					) }
+					{ actions && (
+						<div className="gla-account-card__actions">
+							{ actions }
+						</div>
 					) }
-				</Flex>
+				</div>
 			</Section.Card.Body>
 			{ children }
 		</Section.Card>
diff --git a/js/src/components/account-card/index.scss b/js/src/components/account-card/index.scss
index e33e2b5fd..4a0b96ffa 100644
--- a/js/src/components/account-card/index.scss
+++ b/js/src/components/account-card/index.scss
@@ -5,18 +5,60 @@
 		opacity: 0.5;
 	}
 
+	&--is-expanded-detail {
+		.gla-account-card__indicator {
+			grid-area: 1/3;
+		}
+
+		.gla-account-card__detail {
+			grid-area: 2/2/auto/span 2;
+		}
+	}
+
 	&__styled {
 		&--align-top {
 			align-self: flex-start;
 		}
 	}
 
+	&__body-layout {
+		display: grid;
+		grid-template-columns: auto 1fr auto;
+		align-items: center;
+	}
+
 	&__icon {
+		grid-area: 1/1/span 2;
+		margin-right: $grid-unit-20;
 		line-height: 0;
 	}
 
+	&__subject {
+		grid-area: 1/2;
+	}
+
+	&__indicator {
+		grid-area: 1/3/span 2;
+		margin-left: $grid-unit-20;
+
+		&--align-to-detail {
+			grid-area: 2/3;
+			margin-top: $grid-unit-15;
+		}
+	}
+
+	&__detail {
+		grid-area: 2/2;
+		margin-top: $grid-unit-15;
+	}
+
+	&__actions {
+		grid-area: 3/2/auto/span 2;
+		margin-top: $grid-unit-15;
+	}
+
 	&__title {
-		margin-bottom: $grid-unit-05;
+		margin: 0;
 		color: $black;
 	}
 
@@ -25,6 +67,7 @@
 		flex-direction: column;
 		align-items: flex-start;
 		gap: 1em;
+		margin-top: $grid-unit-05;
 		color: $gray-900;
 
 		> p {
@@ -65,11 +108,12 @@
 	}
 
 	@media (max-width: $break-small) {
-		.components-card__body > .components-flex {
+		&__body-layout {
+			display: flex;
 			flex-direction: column;
 			align-items: flex-start;
-			> .components-flex-block,
-			> .components-flex-item {
+
+			> div {
 				margin: $grid-unit-10;
 			}
 		}

The following git diff result can demonstrate roughly how this solution would work with the current implementation in this PR.

diff --git a/js/src/components/google-combo-account-card/connect-ads/connect-ads.js b/js/src/components/google-combo-account-card/connect-ads/connect-ads.js
index 537526e0e..49e1a9918 100644
--- a/js/src/components/google-combo-account-card/connect-ads/connect-ads.js
+++ b/js/src/components/google-combo-account-card/connect-ads/connect-ads.js
@@ -13,9 +13,11 @@ import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices';
 import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
 import { useAppDispatch } from '.~/data';
 import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';
-import ConnectAccountCard from '../connect-account-card';
+import AdsAccountSelectControl from '.~/components/ads-account-select-control';
+import ConnectButton from '.~/components/google-ads-account-card/connect-ads/connect-button';
+import LoadingLabel from '.~/components/loading-label/loading-label';
+import ConnectedIconLabel from '.~/components/connected-icon-label';
 import ConnectAdsFooter from './connect-ads-footer';
-import ConnectAdsBody from './connect-ads-body';
 
 /**
  * ConnectAds component renders an account card to connect to an existing Google Ads account.
@@ -89,27 +91,47 @@ const ConnectAds = () => {
 		return null;
 	}
 
+	const getIndicator = () => {
+		if ( isLoading ) {
+			return (
+				<LoadingLabel
+					text={ __( 'Connecting…', 'google-listings-and-ads' ) }
+				/>
+			);
+		}
+		if ( isConnected ) {
+			return <ConnectedIconLabel />;
+		}
+		return (
+			<ConnectButton
+				handleConnectClick={ handleConnectClick }
+				value={ value }
+			/>
+		);
+	};
+
 	return (
-		<ConnectAccountCard
-			className="gla-google-combo-service-account-card--ads"
+		<AccountCard
+			className="gla-google-combo-account-card gla-google-combo-service-account-card--ads"
 			title={ __(
 				'Connect to existing Google Ads account',
 				'google-listings-and-ads'
 			) }
-			helperText={ __(
+			helper={ __(
 				'Required to set up conversion measurement for your store.',
 				'google-listings-and-ads'
 			) }
-			body={
-				<ConnectAdsBody
-					isConnected={ isConnected }
-					handleConnectClick={ handleConnectClick }
-					isLoading={ isLoading }
-					setValue={ setValue }
+			alignIndicator="toDetail"
+			indicator={ getIndicator() }
+			detail={
+				<AdsAccountSelectControl
 					value={ value }
+					onChange={ setValue }
+					autoSelectFirstOption
+					nonInteractive={ isConnected }
 				/>
 			}
-			footer={ <ConnectAdsFooter isConnected={ isConnected } /> }
+			actions={ <ConnectAdsFooter isConnected={ isConnected } /> }
 		/>
 	);
 };
diff --git a/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss b/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss
index d68a6f461..fd8960ac3 100644
--- a/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss
+++ b/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss
@@ -8,3 +8,10 @@
 .gla-google-combo-service-connected-icon-label {
 	width: auto;
 }
+
+// This CSS would be better placed in index.scss file and be imported from index.js
+.gla-google-combo-account-card {
+	.gla-account-card__actions .app-button {
+		margin-left: calc(var(--main-gap) / -2);
+	}
+}

📌 Please note that these code changes still need to be polished.

A sample use I temporarily did locally

[joemcgill]

Beyond that, I would like to propose modifying the AccountCard component so that it supports the layout required by GoogleComboAccountCard, in place of the ConnectAccountCard component in this PR and the respective customized implementations in the related PRs.

@eason9487 this is a good idea in general, but not something that we considered necessary to implement the designs for #2509. At this point, a bigger refactor of this component would also affect #2597, where we would also need to consider cases where the AccountCard used to connect/edit existing MC accounts makes use of other components that render AccountCard components that I think would result in rendering nested Card components.

I'd hope that we can get this new GoogleComboAccountCard component merged using the current structure proposed in this PR and #2639 and consider larger structural refactors of the AccountCard component separately.

[eason9487]

Thanks for the work. LGTM.