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

Hook up the Setup Component CTA to create/connect the property and measurement ID #5277

Closed
techanvil opened this issue May 25, 2022 · 7 comments
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented May 25, 2022

Feature Description

Call the GA4 submitChanges function when the Setup Component CTA is clicked. This function will create the property and/or measurement ID and connect them.

For the "no existing GA4 property" variants, i.e when the CTA is Create property, ensure the GA4 propertyID is set to PROPERTY_CREATE prior to calling submitChanges.

Modify the BannerNotification component to allow the Button with Spinner to be used for the CTA button and show the spinner while the API calls made by submitChanges are in progress.

If an error occurs, display the error message in the banner.


Acceptance criteria

  • When the Setup Component CTA is clicked:
    • The property and measurement ID should be connected.
    • For the "no existing GA4 property", or when Set up a new property is selected from the dropdown, the property and measurement ID should first be created.
    • When there is otherwise no measurement ID it should be created.
    • The "use snippet" setting should be persisted (i.e. save the GA4 settings).
    • Implementation note: all the above is taken care of by the existing GA4 submitChanges function.
  • The spinner should be displayed in the button while the above activity is in progress.
  • Any error that occurs should be displayed in the banner according to the design.

Implementation Brief

In assets/js/components/notifications/BannerNotification/index.js:

  • Add a new prop, ctaButton, that composes the CTA button.
  • Render the ctaButton as children if it's available. It renders whatever the element is passed. In this case, it would be a Button with a Spinner reusable component, which is being implemented in Create Button with Spinner component #5271.
  • Otherwise, render the Button and the Spinner components as it is today.
    { ctaLink && (
    <Button
    className="googlesitekit-notification__cta"
    href={ ctaLink }
    target={ ctaTarget }
    onClick={ handleCTAClick }
    disabled={ isAwaitingCTAResponse }
    >
    { ctaLabel }
    </Button>
    ) }
    <Spinner isSaving={ isAwaitingCTAResponse } />

The Setup Component is being Implemented in #5270 and #5275. Add the following logic to the Setup Component:

  • Create a local state error to store the error object.
  • Create a handleSubmitChanges async callback using the useCallback hook with the following:
    • It should submit the changes by dispatching the submitChanges() action of the MODULES_ANALYTICS_4 store.
    • Get the error object from the submitChanges action result.
    • If there is an error, set the error object to the error state.
  • If there is an error, display the error message in the banner by rendering the ErrorNotice component and pass the error state to the error prop.
  • Compose the "Button with Spinner" component, which is being implemented in Create Button with Spinner component #5271, and pass the handleSubmitChanges callback to the onClick prop.
  • Pass the above to the ctaButton prop of the BannerNotification component. This will be rendered as the CTA button.

Test Coverage

  • No new tests are to be added.

QA Brief

  • Ensure that the ga4ActivationBanner feature flag is activated.
  • We need to check two different scenarios:
    • No GA4 property available.
    • GA4 properties exist, but not connected.
  • To test No GA4 property available:
    • Configure the Analytics module in Site Kit - use an account which does not have GA4 properties associated with it.
    • Go to the Site Kit Dashboard.
    • Click on Set up now in the displayed GA4 Activation Banner.
    • Under the No existing Google Analytics 4 property found heading, if Create Property is clicked, it should show a spinner and create a new GA4 property and measurement ID.
  • To test GA4 properties exist, but not connected:
    • Configure the Analytics module in Site Kit - use an account which does have GA4 properties associated with it.
    • Go to the Site Kit Dashboard.
    • Click on Set up now in the displayed GA4 Activation Banner.
    • Under the Connect the Google Analytics 4 property that’s associated with your existing Universal Analytics property heading, selecting a property and clicking on Connect should show a spinner and create a new GA4 property and measurement ID.

Changelog entry

  • Enable the GA4 Activation Banner create/connect a property and measurement ID.
@techanvil techanvil added the Type: Feature New feature label May 25, 2022
@FlicHollis FlicHollis added the P1 Medium priority label May 27, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 29, 2022
@hussain-t hussain-t self-assigned this Jul 18, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jul 22, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Aug 3, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 5, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@tofumatt tofumatt assigned nfmohit and unassigned tofumatt Sep 1, 2022
@nfmohit nfmohit assigned techanvil and unassigned nfmohit Sep 2, 2022
@techanvil techanvil assigned nfmohit and unassigned techanvil Sep 2, 2022
@nfmohit nfmohit assigned techanvil and unassigned nfmohit Sep 2, 2022
@techanvil
Copy link
Collaborator Author

techanvil commented Sep 5, 2022

Update: I've realised that this issue should be covering the case where an existing property does exist too. The implementation should not need to change for this, but I will hold off merging the PR for this issue until #5793 has been merged so we can confirm that it does indeed work for both cases. Additionally the QAB will need to be updated to cover the "existing property" scenario as well (see the updated QAB for #5276).

@techanvil techanvil assigned nfmohit and unassigned techanvil Sep 5, 2022
@nfmohit nfmohit assigned techanvil and unassigned nfmohit Sep 6, 2022
techanvil added a commit that referenced this issue Sep 6, 2022
Create property and measurement ID from SetupBanner
@techanvil techanvil removed their assignment Sep 6, 2022
@wpdarren wpdarren self-assigned this Sep 6, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Sep 6, 2022

QA Update: ⚠️

@techanvil I have one observation, which is linked with the #5276 issue.

  • To test No GA4 property available:

When I click on Create Property the spinner appears within the button, but I see a flicker of the screen with the GA4 dropdown box for a second or two as you can see in the screencast below. It does create GA4 property and the correct splash screen appears when it has been created. Just wondered your thoughts on the screen flicker.

ga4-1.mp4

@wpdarren wpdarren assigned techanvil and unassigned techanvil Sep 6, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Sep 6, 2022

QA Update: ✅

The flicker issue reported above will be fixed as part of 5276, so moving this to approved as the test pass QAB.

Verified:

No GA4 property available:

  • Under the No existing Google Analytics 4 property found heading, if Create Property is clicked, it shows a spinner and creates a new GA4 property and measurement ID.

GA4 properties exist, but not connected:

  • Under the Connect the Google Analytics 4 property, selecting a property and clicking on Connect shows a spinner and creates a new GA4 property and measurement ID.
Screencasts
ga4-1.mp4
ga4-2.mp4

@wpdarren wpdarren removed their assignment Sep 6, 2022
@felixarntz
Copy link
Member

Approval ⚠️

@wpdarren @nfmohit @techanvil Since part of this will only be fixed in #5276, we can only really approve this one once that is through as well. Something we have already noticed and consider problematic is that there is a progress bar showing up after clicking the button. This should not really need to be there, as the button itself already has the loading indicator, so we don't need another loading indicator.

@wpdarren
Copy link
Collaborator

wpdarren commented Sep 9, 2022

@felixarntz

Something we have already noticed and consider problematic is that there is a progress bar showing up after clicking the button. This should not really need to be there, as the button itself already has the loading indicator, so we don't need another loading indicator.

The initial issue was that the screen flickers when the button is created, so this progress bar was introduced because of that. Since 5276 is in QA, should both of these tickets but looked at because the progress bar does appear on screen after clicking on the button in the different states.

c.c. @nfmohit @techanvil

@felixarntz
Copy link
Member

Approval ✅

@wpdarren Tested this again on my side, UI looks good to me now. There no longer is the progress bar together with the button spinner when creating a new GA4 property.

@aaemnnosttv aaemnnosttv added Type: Enhancement Improvement of an existing feature and removed Type: Feature New feature labels Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants