Skip to content
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

Update AppSelectControl when only one account is available #2593

Open
2 tasks
Tracked by #2509
joemcgill opened this issue Sep 6, 2024 · 3 comments · Fixed by #2608
Open
2 tasks
Tracked by #2509

Update AppSelectControl when only one account is available #2593

joemcgill opened this issue Sep 6, 2024 · 3 comments · Fixed by #2608
Assignees

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Sep 6, 2024

Related to of #2509

To provide a consistent user experience in all places where a user might be selecting a Google account to connect from a list of existing accounts, we'll update AppSelectControl that is used in both MerchantCenterSelectControl and AdsAccountSelectControl so that any time there is only one account to select from, that account will immediately shown and the select/dropdown UI will be hidden.

Screenshot:
Image

Acceptance Criteria

  • When either MerchantCenterSelectControl or AdsAccountSelectControl components are rendered with only one account, no dropdown icon is shown and the account is automatically selected.
  • When either MerchantCenterSelectControl or AdsAccountSelectControl components are rendered with more than one account, the current SelectControl behavior is retained (i.e., users can select the account from a dropdown, and if a value is already set, that value will be selected).

Implementation Brief

Per this suggestion, The current AppSelectControl component should be updated to be able to show a simplified UI whenever there is only one account to connect. The simplified UI should be controllable by a new selectSingleValue prop that defaults to false.

The MerchantCenterSelectControl and AdsAccountSelectControl components will both be updated to pass selectSingleValue:true.

The AdsAccountSelectControl is currently located in /js/src/components/google-ads-account-card/connect-ads/ads-account-select-control.js, as part of the GoogleAdsAccountCard component. Since this will be reused, it should be moved to js/src/components to be consistent with the location of the MerchantCenterSelectControl.

A nice to have would be to make these two controls consistent in that if no accounts are passed to the AdsAccountSelectControl component, then it will make use of the useExistingGoogleAdsAccounts() hook to automatically populate the dropdown, similar to the way MerchantCenterSelectControl handles this here, as this consistency should make reusing this control in the new GoogleComboAccountsCard component easier to implement.

Test Coverage

  • Existing Jest tests for either component should be updated to pass if needed.

Definition Questions

Is there a reason that the logic between MerchantCenterSelectControl and AdsAccountSelectControl is not consistent? For example, the MC version handles fetching the existing MC accounts, sorting and setting the initial value (ref) while the Ads version requires the account fetching logic to happen by the consuming component.

@joemcgill joemcgill changed the title Update the AdsAccountSelectControl for reuse Update MerchantCenterSelectControl and AdsAccountSelectControl to use AppSelectorTextControl Sep 6, 2024
@joemcgill joemcgill added the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Sep 6, 2024
@joemcgill
Copy link
Collaborator Author

@eason9487 and @asvinb I've stubbed this out as a follow up to #2592. Would appreciate your thoughts.

@eason9487
Copy link
Member

Is there a reason that the logic between MerchantCenterSelectControl and AdsAccountSelectControl is not consistent?

They were more consistent in their initial implementation. I don't recall any particular reason for this. They should have just been changed individually as per the later requirements. See:

@joemcgill joemcgill changed the title Update MerchantCenterSelectControl and AdsAccountSelectControl to use AppSelectorTextControl Update AppSelectControl when only one account is availalbe. Sep 9, 2024
@joemcgill joemcgill removed the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Sep 9, 2024
@joemcgill joemcgill removed their assignment Sep 9, 2024
@dsawardekar dsawardekar self-assigned this Sep 11, 2024
@dsawardekar dsawardekar changed the title Update AppSelectControl when only one account is availalbe. Update AppSelectControl when only one account is available Sep 11, 2024
@joemcgill joemcgill linked a pull request Sep 13, 2024 that will close this issue
@asvinb asvinb assigned asvinb and unassigned joemcgill Sep 16, 2024
@asvinb
Copy link
Collaborator

asvinb commented Sep 17, 2024

@joemcgill I made a few updates to the PR. Can you kindly take a look please?
Thanks!

@asvinb asvinb assigned joemcgill and ankitrox and unassigned asvinb and joemcgill Sep 17, 2024
@ankitrox ankitrox assigned asvinb and unassigned ankitrox Sep 18, 2024
@asvinb asvinb assigned ankitrox and unassigned asvinb Sep 18, 2024
@asvinb asvinb assigned ankitguptaindia and joemcgill and unassigned asvinb and joemcgill Sep 23, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Sep 30, 2024
@asvinb asvinb assigned eason9487 and unassigned asvinb Sep 30, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Oct 1, 2024
@asvinb asvinb assigned eason9487 and asvinb and unassigned asvinb and eason9487 Oct 1, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Oct 4, 2024
@asvinb asvinb assigned eason9487 and unassigned asvinb Oct 4, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Oct 7, 2024
@asvinb asvinb assigned eason9487 and unassigned asvinb Oct 7, 2024
@eason9487 eason9487 removed their assignment Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants