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

Creation of <WooCommerceRedirectModal> Modal Component #10172

Open
23 tasks
10upsimon opened this issue Feb 5, 2025 · 8 comments
Open
23 tasks

Creation of <WooCommerceRedirectModal> Modal Component #10172

10upsimon opened this issue Feb 5, 2025 · 8 comments
Labels
javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Feb 5, 2025

Feature Description

The WooCommerce redirect modal component is the core element within the WooCommerce redirect epic, and is the only user facing element that will give users the option of using (or continuing to use) Google for WooCommerce or to proceed with Site Kit for Ads module and potentially Ads account and campaign creation.

Said modal will present itself in 3 possible scenarios that a user may find themselves in when opting to proceed with Ads module set up in Site Kit:

Scenario 01 - WooCommerce is installed and active, but Google for WooCommerce is not

image.png

The modal will give the user the option using Google for WooCommerce, with a primary CTA link navigating to the WordPress dashboard plugin directory page, pre-filtered so that Google for WooCommerce is first in the list.

Scenario 02 - WooCommerce & Google for WooCommerce are installed and active, but no Ads account is linked

image.png

The same modal as in the prior scenario will be used, with the exception that the primary cta link will instead navigate to the Google for WooCommerce merchant dashboard page, from where the user may begin Ads account setup via Google for WooCommerce.

Scenario 03 - WooCommerce & Google for WooCommerce are installed and active, and an Ads account linked to Google for WooCommerce is detected

image.png

The user is made aware that an existing account via Google for WooCommerce is detected, giving them the option to continue with said existing account (linking to the Google for WooCommerce merchant overview dashboard) or continue to create an account via Site Kit.

Modal Components/Elements

Each variation of the modal, and thus the modal component overall, will consist of the following elements:

Modal Dismissal

All instances of the modal, regardless of the scenario at hand, should be deemed "dismissed" if either the primary or secondary CTA's are clicked. Dismissals should be valid for the duration of the users session, and make use of api/cache logic currently in the codebase.

For a more detailed breakdown of the modal, please see the WooCommerce Redirect Modal Component (UI) section of the design doc.

Figma designs can be found here.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • A new modal component, WooCommerceRedirectModal is created matching the Figma designs
  • Said modal should be triggered when setup Ads module is triggered - currently from setup CTA #10175 and within the new Ads setup screen (see Updates to the Ads Module <SetupMainPAX> Component to Accommodate New Layout #10193 ) - when one of the following conditions are fulfilled:
  • Primary CTA should behave in following way:
    • When WooCommerce is installed and active, but Google for WooCommerce is not
      • Navigate to the WordPress dashboard plugin directory page, pre-filtered so that Google for WooCommerce is first in the list.
    • When WooCommerce & Google for WooCommerce are installed and active, but no Ads account is linked
      • Navigate to the Google for WooCommerce merchant dashboard page, from where the user may begin Ads account setup via Google for WooCommerce.
  • Primary CTA in both cases opens within same tab (even it has external icon, it indicates that it takes you to the external service outside the Site Kit)
    • Ads setup flow should be fully interrupted, so module does not enter the active state
  • Secondary CTA Continue with Site Kit in both cases continues with the Ads module setup flow
  • Clicking any of the CTA's in both cases should dismiss the modal for the current user session
  • See UI Modal section of the design doc

Implementation Brief

  • Update assets/js/googlesitekit/datastore/site/cache.js

    • Include new resolver for getCacheItem, that will act as a wrapper for retrieving the value using getItem from the assets/js/googlesitekit/api/cache.js
  • Add assets/js/modules/ads/components/woocommerce/WooCommerceRedirectModal.js

    • It should accept two props:
      • dialogActive - Bool, controls the visible state of the dialog
      • onDismiss - Func, optional, accepts a callback function for when the modal is dismissed
    • Implement the <Dialog>, <DialogTitle>, <DialogContent>, and <DialogFooter> components as per other implementations of modals in the codebase and content matching the referenced Figma designs. You may use the modal at assets/js/components/KeyMetrics/ConfirmSitePurposeChangeModal.js as an example.
      • Forward the dialogActive prop to the open prop of the <Dialog> component
      • Forward the onDismiss prop to the onClose prop of the <Dialog> component
    • Create onClose callback function that should dispatch setCacheItem action from CORE_SITE datastore, you can use wc-redirect-modal for key. Use 0 for ttl option, so value is preserved in the local storage until session is cleared.
      • Invoke onDismiss callback passed as a prop, if present
    • Create a callback function that will be triggered when the primary CTA is clicked, i.e navigateToGoogleForWooCommerce(), this callback function should:
      • Navigate to the URI obtained from getGoogleForWooCommerceRedirectURI selector from CORE_SITE datastore. For navigating, use navigateTo action from CORE_LOCATION store.
      • Invoke onClose callback
    • Create a callback function that will be triggered when the secondary CTA is clicked, this callback function should:
      • Enter the module activation state, by invoking the callback from useActivateModuleCallback hook (pass the module slug parameter - ads)
      • Invoke the onClose callback
    • In the <DialogFooter> component, add two button components:
      • A <Button> component with prop tertiary, labeled "Continue with Site Kit"
        • Call the onSecondaryCTAClickCallback callback function as the onClick prop value
      • A <SpinnerButton> component labelled Use Google for WooCommerce
        • Call the navigateToGoogleForWooCommerce callback function
        • Use isNavigating selector from CORE_LOCATION store in primary button callback, to flag that the navigating is happening, and pass the value to the isSaving prop
    • Pass onClose callback to the onClose prop to the Dialog component
  • In assets/js/modules/ads/components/setup/SetupMainPAX.js

Test Coverage

  • Create the necessary VRT story for the new modal component
  • Add basic test coverage for the new modal - verifying that it is triggered when conditions are met for example

QA Brief

  • Verify that modal story ?path=/story/modules-ads-woocommerceredirectmodal--default is resembling the one in Figma (link referenced in the AC)
  • Few notes:
    • The points from AC about redirection on button click cannot be reproduced in the story, this will be tested when modal is included in other issues. The functionality itself is tested via JS tests, and will be covered during CR and tests passing in CI
    • Font weight for the heading renders a bit thicker in the browser vs figma
    • All modals in implementation open with a focus on the first button, in Figma preview is shown without focus, to defocus the button, click anywhere within the modal

Changelog entry

  • Add the WooCommerceRedirectModal component.
@10upsimon 10upsimon added javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature labels Feb 5, 2025
@10upsimon 10upsimon self-assigned this Feb 5, 2025
@10upsimon 10upsimon assigned zutigrm and unassigned 10upsimon Feb 7, 2025
@zutigrm zutigrm assigned 10upsimon and unassigned zutigrm Feb 7, 2025
@10upsimon
Copy link
Collaborator Author

Noting that the triggering specific issue at #10178 was closed out in favor of this issue which also outlines trigger rules.

@10upsimon
Copy link
Collaborator Author

@zutigrm LGTM, moving to IB and assigning myself

@zutigrm
Copy link
Collaborator

zutigrm commented Feb 12, 2025

Thanks @10upsimon I have simplified and expanded the IB to cover the interruption of the setup and modal inclusion, with few other tweaks

@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm Feb 12, 2025
@aaemnnosttv
Copy link
Collaborator

Thanks both, I have a few questions/concerns about the suggested approach.

googlesitekit/modules/datastore/modules.js

  • In registerModule action, introduce new optional setting, onSetupModule, and pass it to the payload

The name of this action is a bit ambiguous considering it is an action that seems to be intended for the purpose of interrupting/preventing activation. As behavior we want to be defined at the module level, I'd suggest we follow a similar pattern to canActivateModule (and its resolver) which also seems like it has some overlap with the purpose here.

settings/SettingsInactiveModules.js

  • Render the WooCommerceRedirectModal within Layout wrapper component, and use the dialogActive prop to trigger then modal

I'd like to find a different place to include this if possible as it seems a bit too module-specific to bake into a core component like SettingsInactiveModules.

I'll think about this a bit more and discuss with @10upsimon later today to have this ready for tomorrow.

@10upsimon 10upsimon assigned 10upsimon and unassigned aaemnnosttv Feb 14, 2025
@10upsimon
Copy link
Collaborator Author

@zutigrm IB ✅ , moving to EB!

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 20, 2025
@zutigrm zutigrm added the P0 High priority label Feb 21, 2025
@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 21, 2025
@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 21, 2025
@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Feb 24, 2025
@eugene-manuilov eugene-manuilov removed their assignment Feb 24, 2025
@kelvinballoo kelvinballoo self-assigned this Feb 24, 2025
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Feb 24, 2025

QA Update ⚠

I have 2 observations (albeit quite minor) and a clarification:

ITEM 1:
The font weight for the button is 500 on Figma whereas it's 400 on our implementation.
If this is intentional to match other modals on the plugin, we can ignore this.

URL : https://google.github.io/site-kit-wp/storybook/develop/?path=/story/modules-ads-woocommerceredirectmodal--default

Image Image

ITEM 2:
The distance between the description text and the button is 30px based on figma:

Image

However, in our implementation, it's 28+5 = 33px:

Image Image

ITEM 3: CLARIFICATION
Looking at the AC, there are some points on the behaviour of the modal.
Based on the notes in the QAB, am I right to say that we are not to test on the behaviour in this ticket and we are only concerned about the modal matching Figma for now?
The behaviour would be tested in another ticket.

@zutigrm
Copy link
Collaborator

zutigrm commented Feb 24, 2025

Thanks @kelvinballoo

ITEM 1:
The font weight for the button is 500 on Figma whereas it's 400 on our implementation.
If this is intentional to match other modals on the plugin, we can ignore this.

Correct, buttons in all modals are 400

ITEM 2:
The distance between the description text and the button is 30px based on figma:

It didn't make sense to override global actions wrapper that generates margin of 2em in all modals, for this case only

ITEM 3: CLARIFICATION
Looking at the AC, there are some points on the behaviour of the modal.

Not testable in the storybook, this will be verified in other issues that implement the modal

@zutigrm zutigrm removed their assignment Feb 25, 2025
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks for the update @zutigrm .

This is verified good in that case and good for approval.

Verified on the visual aspect solely, as per QAB:

  • Verified that modal story ?path=/story/modules-ads-woocommerceredirectmodal--default is resembling the one in Figma
Implementation: Image

Figma:

Image

@kelvinballoo kelvinballoo removed their assignment Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants