2593: Update AppSelectControl to single select #2608

dsawardekar · 2024-09-12T04:36:02Z

Changes proposed in this Pull Request:

This PR updates the AppSelectControl to show a single selection when only one choice is present.

Closes #2593

Screenshots:

Multi-select:

Single-select:

Detailed test instructions:

Reset your environment, and start a fresh onboarding
On Step 1, connect your Google account
In the Merchant and Ads dropdowns you should see a pre-selected option if you have only one choice
If you have multiple choices, the dropdown should still show

Technical decisions:

To maintain consistency with MerchantCenterSelectControl, the Select one label has been removed from AdsAccountSelectControl: https://github.com/woocommerce/google-listings-and-ads/pull/2608/files#diff-3c83d48212438511a41629a8f6b02ac7666ca664d0c25a734497a1416856d115L17
The first option within AdsAccountSelectControl is set as the default value if none is provided (same behaviour as MerchantCenterSelectControl).
Labels have been updated to Connect to an existing account

Changelog entry

Update - Automatically preselect a Google Ads account when there is only one, as well as adjust the UI presentation..

codecov · 2024-09-12T04:38:53Z

Codecov Report

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

Project coverage is 62.7%. Comparing base (345e6b1) to head (d8f3ed9).
Report is 17 commits behind head on feature/2509-consolidate-google-account-cards.

Files with missing lines	Patch %	Lines
js/src/components/app-select-control/index.js	86.7%	2 Missing ⚠️

Additional details and impacted files

@@                              Coverage Diff                              @@
##           feature/2509-consolidate-google-account-cards   #2608   +/-   ##
=============================================================================
  Coverage                                           62.7%   62.7%           
=============================================================================
  Files                                                319     319           
  Lines                                               5063    5071    +8     
  Branches                                            1233    1237    +4     
=============================================================================
+ Hits                                                3173    3179    +6     
- Misses                                              1716    1718    +2     
  Partials                                             174     174

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

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

Files with missing lines	Coverage Δ
...src/components/ads-account-select-control/index.js	`100.0% <100.0%> (ø)`
...nents/google-ads-account-card/connect-ads/index.js	`96.0% <ø> (ø)`
...components/merchant-center-select-control/index.js	`100.0% <100.0%> (ø)`
js/src/components/app-select-control/index.js	`88.2% <86.7%> (-11.8%)`	⬇️

dsawardekar · 2024-09-12T04:56:10Z

@joemcgill I have implemented the changes, but this needs a UX revisit due to the way the components are structured. The Label for the AppSelectControl is outside, and the options are only computed in the parent AdsAccountSelectControl, etc. The label will still say 'Select an existing account' even when we have preselected the option.

https://github.com/woocommerce/google-listings-and-ads/blob/update/2593-update-app-select-control-single-select/js/src/components/google-ads-account-card/connect-ads/index.js#L96

cc @asvinb

asvinb · 2024-09-12T18:17:11Z

@dsawardekar @joemcgill In that case maybe we need to rename the label to what we have in the new designs which is more generic and it prompts the user to connect an account instead of selecting an account:

Connect to existing Google Ads account

joemcgill · 2024-09-13T13:20:38Z

The Label for the AppSelectControl is outside, and the options are only computed in the parent AdsAccountSelectControl, etc. The label will still say 'Select an existing account' even when we have preselected the option.

Thanks for flagging this @dsawardekar. When we use this control in the new GoogleComboAccountsCard in #2596 or #2597, we will use a more appropriate label. In the current GoogleAdsAccountCard and GoogleMCAccountCard components that share these controls, the should still use the current behavior, since we're not setting selectSingleValue to true by default.

joemcgill

Small nitpick, but looks good.

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

joemcgill · 2024-09-13T16:52:36Z

In the current GoogleAdsAccountCard and GoogleMCAccountCard components that share these controls, the should still use the current behavior, since we're not setting selectSingleValue to true by default.

After testing this PR, I think I misunderstood the concern. Since this sets selectSingleValue to true in both the MerchantCenterSelectControl and AdsAccountSelectControl components, this will affect the current places where those controls are used. We could also add the ability to set the selectSingleValue prop on those components to be able to retain the current behavior for the GoogleAdsAccountCard and GoogleMCAccountCard components, but I think I prefer @asvinb's suggestion that we update those components so the text reads "Connect to an existing account." rather than "Select and existing account".

Example:

ankitrox

Thanks for the excellent work here. I have tested the functionality and it seems to be working fine. I have added few comments that needs to be addressed or may be needs to looked into.

ankitrox · 2024-09-18T13:54:44Z

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

+			className={ classNames( 'app-select-control', className, {
+				'app-select-control--has-single-value': showSingleValue,
+			} ) }


It's better to prefix the class names with gla or g4w

@ankitrox I see there are other parts where the app- prefix it used. For e.g most of the js/src/components/app-* components. Let's keep it as is.
Examples: https://github.com/search?q=repo%3Awoocommerce%2Fgoogle-listings-and-ads+.app-+language%3ASCSS&type=code&l=SCSS

Please check this comment from Eason.

I understand what you're saying @ankitrox , but if you look at the current codebase, there are components which have thegla-* prefix class names and other specific components com which have the app-* prefix. Clearly there is a reason to why this is so. In this particular case, we are editing an existing component which already uses the app-* prefix, so I would be reluctant to change it. If you look at

google-listings-and-ads/js/src/components/shipping-rate-section/minimum-order-card/minimum-order-card.scss

Line 3 in 24044c5

width: $gla-width-medium;

, you'll notice that it's using both, gla and app prefix. I think the base components used within the app are prefixed with app-*. @eason9487 Can you shed some light please, about why we have 2 different CSS prefix, i.e gla-* and app-*? Thanks!

Early components often came with an app- prefix, but that is unnecessary and can be omitted for new components as it becomes a bit verbose.

Regarding the CSS class naming, the main reason is to reduce the chance of CSS naming collisions. app- has significantly higher generality, so gla- or g4w- would be more appropriate. The current app- uses are almostly carry-over from earlier times, and newer code would be better to avoid using it as a prefix.

If it involves changing existing code, it is not necessary to modify it. However, if the change (scope) is minor, it would be appreciated if it could be adjusted in the meantime.

Thanks for the clarification @eason9487 . I think that helps a lot. For the scope of this ticket, I suggest we keep it as is. We can have a separate ticket to adjust the class name. 👍

js/src/components/app-select-control/index.scss

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

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

ankitrox

@asvinb I have added only couple of comments, but you can decide whether to address those. I am approving this PR as other changes are looking fine to me.

Thanks

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

ankitrox · 2024-09-20T13:59:43Z

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

+			className={ classNames( 'app-select-control', className, {
+				'app-select-control--has-single-value': showSingleValue,
+			} ) }


Please check this comment from Eason.

…date-app-select-control-single-select

ankitguptaindia · 2024-09-23T12:19:44Z

QA Status: ⚠️ Needs feedback

In the Merchant and Ads dropdowns you should see a pre-selected option if you have only one choice
If you have multiple choices, the dropdown should still show

@asvinb
As per the requirement, if a Google account has only one Ads or Merchant Center (MC) account, it should automatically pre-select that account. However, if multiple accounts exist, the dropdown should display the 'Select One' option by default.

Currently, it shows the first Ads and MC account in the dropdown as autoselected even when multiple sub-accounts are present in the Google account.

Please refer to the screencast below:

Recording.859.mp4

asvinb · 2024-09-23T19:52:50Z

@ankitguptaindia I added a note in the "Technical decisions":

To maintain consistency with MerchantCenterSelectControl, the Select one label has been removed from AdsAccountSelectControl: https://github.com/woocommerce/google-listings-and-ads/pull/2608/files#diff-3c83d48212438511a41629a8f6b02ac7666ca664d0c25a734497a1416856d115L17

@joemcgill What do you think? It's mostly making the MerchantCenterSelectControl and AdsAccountSelectControl consistent and MerchantCenterSelectControl did not have the "Select one" option.

joemcgill · 2024-09-26T14:23:55Z

I think making these consistent sounds reasonable to me. So if none are already selected the first one in the list becomes the default, since that’s how the MC field already worked?

asvinb · 2024-09-26T14:24:57Z

Yes @joemcgill that's how the MC field works. @ankitguptaindia Can you confirm please?

ankitguptaindia · 2024-09-27T11:49:54Z

QA/Test Report- ✅

Testing Environment -

WordPress: 6.6.2
Theme active on store: Twenty Twenty-Four Version: 1.2
WooCommerce - Version 9.3.3
PHP: 8.3
Web Server: Nginx
Browser: Chrome - Version 129
OS: macOS Sonoma 14.6.1

Test Results - The PR is functioning as expected.

✅ When no Google/MC accounts are available, it prompts to create new accounts:
✅ When only one Google/MC account is associated with the Google Account, it should automatically select that account. No drop-down should appear.

✅ When multiple Google/MC accounts are available, it should select the first account and display a drop-down to choose other accounts.
✅ When a Google account has multiple MC accounts but only a single Ad account, it should show a drop-down for MC but none for Ads.

Functional Demo / Screencast -

Googler.accounts.drop.down.876.mp4

eason9487

Thanks for the work. Left a few comments about the possibility of simplifying code and improving consistency.

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

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

…ate/2593-update-app-select-control-single-select

joemcgill · 2024-09-30T19:41:41Z

Just noticed that this PR should be pointing at the feature/2509-consolidate-google-account-cards branch. I've update the base branch and resolved conflicts in dc9b152

No longer applicable

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

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

js/src/components/merchant-center-select-control/index.js

eason9487

Found two new issues and it would be better to address them to avoid potential issues in the future.

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

js/src/components/merchant-center-select-control/index.js

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

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

js/src/components/merchant-center-select-control/index.js

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

First pass at appselectcontrol changes

e7e6687

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Sep 12, 2024

dsawardekar changed the base branch from develop to feature/2458-streamline-onboarding September 12, 2024 04:36

Fix linter warnings

3b87196

joemcgill linked an issue Sep 13, 2024 that may be closed by this pull request

Update AppSelectControl when only one account is available #2593

Open

2 tasks

joemcgill previously requested changes Sep 13, 2024

View reviewed changes

js/src/components/app-select-control/index.js Outdated Show resolved Hide resolved

asvinb added 3 commits September 17, 2024 20:00

Move AdsAccountSelectControl

5ab1b61

Update Jest tests.

2fb0353

Update label.

fd9f50a

asvinb marked this pull request as ready for review September 17, 2024 20:09

asvinb changed the title ~~Update/2593 update app select control single select~~ 2593: Update AppSelectControl to single select Sep 17, 2024

Fix e2e tests.

a17ce78

ankitrox requested changes Sep 18, 2024

View reviewed changes

Do not use translation.

6342061

ankitrox approved these changes Sep 20, 2024

View reviewed changes

Merge branch 'feature/2458-streamline-onboarding' into update/2593-up…

f0e12a7

…date-app-select-control-single-select

ankitguptaindia self-requested a review September 27, 2024 12:04

ankitguptaindia approved these changes Sep 27, 2024

View reviewed changes

eason9487 reviewed Sep 30, 2024

View reviewed changes

js/src/components/app-select-control/index.js Outdated Show resolved Hide resolved

js/src/components/ads-account-select-control/index.js Outdated Show resolved Hide resolved

js/src/components/ads-account-select-control/index.js Outdated Show resolved Hide resolved

Move auto select logic to AppSelectControl.

d575ba6

joemcgill changed the base branch from feature/2458-streamline-onboarding to feature/2509-consolidate-google-account-cards September 30, 2024 19:35

Merge branch 'feature/2509-consolidate-google-account-cards' into upd…

dc9b152

…ate/2593-update-app-select-control-single-select

eason9487 reviewed Oct 1, 2024

View reviewed changes

asvinb added 2 commits October 1, 2024 15:37

Address CR feedback.

0eafa45

Use useRef to prevent re renders.

8c43ac6

eason9487 reviewed Oct 4, 2024

View reviewed changes

asvinb added 3 commits October 4, 2024 15:42

Check for undefined values.

004c9f6

Make better use of useRef.

08b16e0

Updated JSDocs.

f290352

eason9487 reviewed Oct 7, 2024

View reviewed changes

js/src/components/ads-account-select-control/index.js Outdated Show resolved Hide resolved

js/src/components/merchant-center-select-control/index.js Outdated Show resolved Hide resolved

js/src/components/app-select-control/index.js Outdated Show resolved Hide resolved

Fix JSDocs.

d8f3ed9

eason9487 approved these changes Oct 7, 2024

View reviewed changes

asvinb merged commit 7d31e5c into feature/2509-consolidate-google-account-cards Oct 7, 2024
7 checks passed

asvinb deleted the update/2593-update-app-select-control-single-select branch October 7, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2593: Update AppSelectControl to single select #2608

2593: Update AppSelectControl to single select #2608

dsawardekar commented Sep 12, 2024 •

edited by eason9487

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

dsawardekar commented Sep 12, 2024

asvinb commented Sep 12, 2024

joemcgill commented Sep 13, 2024

joemcgill left a comment

joemcgill commented Sep 13, 2024 •

edited

Loading

ankitrox left a comment

ankitrox Sep 18, 2024

asvinb Sep 18, 2024 •

edited

Loading

ankitrox Sep 20, 2024

asvinb Sep 20, 2024

eason9487 Sep 20, 2024

asvinb Sep 30, 2024

ankitrox left a comment

ankitrox Sep 20, 2024

ankitguptaindia commented Sep 23, 2024

asvinb commented Sep 23, 2024

joemcgill commented Sep 26, 2024

asvinb commented Sep 26, 2024

ankitguptaindia commented Sep 27, 2024

eason9487 left a comment

joemcgill commented Sep 30, 2024

eason9487 left a comment

2593: Update AppSelectControl to single select #2608

2593: Update AppSelectControl to single select #2608

Conversation

dsawardekar commented Sep 12, 2024 • edited by eason9487 Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Technical decisions:

Changelog entry

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

dsawardekar commented Sep 12, 2024

asvinb commented Sep 12, 2024

joemcgill commented Sep 13, 2024

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill commented Sep 13, 2024 • edited Loading

ankitrox left a comment

Choose a reason for hiding this comment

ankitrox Sep 18, 2024

Choose a reason for hiding this comment

asvinb Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

ankitrox Sep 20, 2024

Choose a reason for hiding this comment

asvinb Sep 20, 2024

Choose a reason for hiding this comment

eason9487 Sep 20, 2024

Choose a reason for hiding this comment

asvinb Sep 30, 2024

Choose a reason for hiding this comment

ankitrox left a comment

Choose a reason for hiding this comment

ankitrox Sep 20, 2024

Choose a reason for hiding this comment

ankitguptaindia commented Sep 23, 2024

QA Status: ⚠️ Needs feedback

asvinb commented Sep 23, 2024

joemcgill commented Sep 26, 2024

asvinb commented Sep 26, 2024

ankitguptaindia commented Sep 27, 2024

QA/Test Report- ✅

eason9487 left a comment

Choose a reason for hiding this comment

joemcgill commented Sep 30, 2024

eason9487 left a comment

Choose a reason for hiding this comment

dsawardekar commented Sep 12, 2024 •

edited by eason9487

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

joemcgill commented Sep 13, 2024 •

edited

Loading

asvinb Sep 18, 2024 •

edited

Loading