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

Simplify and enhance the loading state control during the Google Ads account creation and claiming #2413

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

It's a pre-processing PR for event tracking to be added for #2215. In this way, the position and timing of adding event tracking could be made more explicit.

  • Simplify the loading state control during the Google Ads account creation and claiming.
    • The reason for removing the "Waiting…" state is that once the user closes all pop-up windows, there is no button to open the invitation window again from the onboarding page unless they refresh the webpage.
  • Parallel fetches Google Ads account and its status in the useUpsertAdsAccount hook to reduce the waiting time a bit.
  • Disable the "Or, use your existing Google Ads account" button while upserting Google Ads account.
    image

Screenshots:

Kapture.2024-05-27.at.17.53.59.mp4

Detailed test instructions:

  1. Disconnect Google Ads account if there is a connected one.
  2. Go to step 1 of the onboarding flow.
  3. Connect a Google Merchant Center account first.
  4. Create a new Google Ads account and claim it.
  5. During Google Ads account creation and claiming, inspect the loading state to see if it's correct and not flashing other unexpected stuff.
  6. Make sure it doesn't have issues like the following video:
    1. The "Or, use your existing Google Ads account" button is enabled while upserting Google Ads account.
    2. There is no way to open the pop-up window again from the page after closing the pop-up window opened by clicking on the "Claim Account"
Kapture.2024-05-27.at.18.02.05.mp4

Changelog entry

@eason9487 eason9487 requested a review from a team May 27, 2024 10:16
@eason9487 eason9487 self-assigned this May 27, 2024
@github-actions github-actions bot added the changelog: tweak Small change, that isn't actually very important. label May 27, 2024
Copy link

codecov bot commented May 27, 2024

Codecov Report

Attention: Patch coverage is 57.89474% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 61.8%. Comparing base (6e54e69) to head (b62983b).

Additional details and impacted files

Impacted file tree graph

@@                        Coverage Diff                         @@
##             tweak/retry-get-merchant-link   #2413      +/-   ##
==================================================================
- Coverage                             64.6%   61.8%    -2.7%     
==================================================================
  Files                                  459     319     -140     
  Lines                                16862    4982   -11880     
  Branches                                 0    1212    +1212     
==================================================================
- Hits                                 10886    3080    -7806     
+ Misses                                5976    1733    -4243     
- Partials                                 0     169     +169     
Flag Coverage Δ
js-unit-tests 61.8% <57.9%> (?)
php-unit-tests ?

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

Files Coverage Δ
...ts/google-ads-account-card/claim-account-button.js 50.0% <100.0%> (ø)
...nts/google-ads-account-card/claim-account/index.js 100.0% <100.0%> (ø)
...s/google-ads-account-card/create-account-button.js 11.1% <100.0%> (ø)
js/src/hooks/useUpsertAdsAccount.js 58.8% <57.1%> (ø)
...mponents/google-ads-account-card/create-account.js 65.6% <37.5%> (ø)

... and 773 files with indirect coverage changes

Base automatically changed from tweak/retry-get-merchant-link to feature/seamlessly-conversion-tracking May 29, 2024 03:36
Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for enhancing the loading state. Tested locally I could confirm the Or, use your existing Google Ads account button is disabled when upserting Google Ads account, and the claiming pop-up window can be opened again after it's closed without successfully claimed.

@eason9487 eason9487 merged commit b50c83e into feature/seamlessly-conversion-tracking May 29, 2024
9 checks passed
@eason9487 eason9487 deleted the tweak/ads-account-create-and-claim branch May 29, 2024 06:27
@mikkamp mikkamp mentioned this pull request Jun 10, 2024
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: tweak Small change, that isn't actually very important.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants