site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Create property and measurement ID from SetupBanner

Open nfmohit opened this issue 3 years ago • 2 comments

Summary

Addresses issue:

  • #5277

Relevant technical choices

This PR adds the property and measurement ID creation mechanism in SetupBanner of GA4 Activation Banner.

PR Author Checklist

  • [ ] My code is tested and passes existing unit tests.
  • [ ] My code has an appropriate set of unit tests which all pass.
  • [ ] My code is backward-compatible with WordPress 4.7 and PHP 5.6.
  • [ ] My code follows the WordPress coding standards.
  • [ ] My code has proper inline documentation.
  • [x] I have added a QA Brief on the issue linked above.
  • [x] I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

  • [ ] Run the code.
  • [ ] Ensure the acceptance criteria are satisfied.
  • [ ] Reassess the implementation with the IB.
  • [ ] Ensure no unrelated changes are included.
  • [ ] Ensure CI checks pass.
  • [ ] Check Storybook where applicable.
  • [ ] Ensure there is a QA Brief.

Merge Reviewer Checklist

  • [ ] Ensure the PR has the correct target branch.
  • [ ] Double-check that the PR is okay to be merged.
  • [ ] Ensure the corresponding issue has a ZenHub release assigned.
  • [ ] Add a changelog message to the issue.

nfmohit avatar Aug 19 '22 14:08 nfmohit

Size Change: +1.07 kB (0%)

Total Size: 1.5 MB

Filename Size Change
./dist/assets/js/googlesitekit-activation-********************.js 28 kB +20 B (0%)
./dist/assets/js/googlesitekit-adminbar-********************.js 36 kB +6 B (0%)
./dist/assets/js/googlesitekit-api-********************.js 9.24 kB -5 B (0%)
./dist/assets/js/googlesitekit-dashboard-details-********************.js 61.1 kB +521 B (+1%)
./dist/assets/js/googlesitekit-dashboard-********************.js 68.9 kB +500 B (+1%)
./dist/assets/js/googlesitekit-dashboard-splash-********************.js 70.8 kB +43 B (0%)
./dist/assets/js/googlesitekit-data-********************.js 2.09 kB +3 B (0%)
./dist/assets/js/googlesitekit-datastore-forms-********************.js 8.88 kB -1 B (0%)
./dist/assets/js/googlesitekit-datastore-ui-********************.js 8.97 kB -3 B (0%)
./dist/assets/js/googlesitekit-datastore-user-********************.js 30.3 kB -4 B (0%)
./dist/assets/js/googlesitekit-idea-hub-post-list-********************.js 26.1 kB +17 B (0%)
./dist/assets/js/googlesitekit-modules-adsense-********************.js 69.6 kB -57 B (0%)
./dist/assets/js/googlesitekit-modules-analytics-4-********************.js 19.1 kB +1 B (0%)
./dist/assets/js/googlesitekit-modules-analytics-********************.js 68.2 kB +9 B (0%)
./dist/assets/js/googlesitekit-modules-idea-hub-********************.js 27.6 kB +1 B (0%)
./dist/assets/js/googlesitekit-modules-optimize-********************.js 19.4 kB +9 B (0%)
./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js 18.5 kB +5 B (0%)
./dist/assets/js/googlesitekit-modules-search-console-********************.js 38.5 kB -5 B (0%)
./dist/assets/js/googlesitekit-modules-tagmanager-********************.js 32.1 kB +2 B (0%)
./dist/assets/js/googlesitekit-modules-thank-with-google-********************.js 24.8 kB -4 B (0%)
./dist/assets/js/googlesitekit-polyfills-********************.js 379 B -1 B (0%)
./dist/assets/js/googlesitekit-settings-********************.js 50.1 kB +53 B (0%)
./dist/assets/js/googlesitekit-user-input-********************.js 44.9 kB -8 B (0%)
./dist/assets/js/googlesitekit-vendor-********************.js 323 kB +2 B (0%)
./dist/assets/js/googlesitekit-widgets-********************.js 19.6 kB +3 B (0%)
./dist/assets/js/googlesitekit-wp-dashboard-********************.js 59.8 kB -41 B (0%)
ℹ️ View Unchanged
Filename Size
./dist/assets/css/googlesitekit-admin-css-********************.min.css 48.4 kB
./dist/assets/css/googlesitekit-adminbar-css-********************.min.css 11.1 kB
./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css 5.97 kB
./dist/assets/js/31-********************.js 2.8 kB
./dist/assets/js/32-********************.js 2.28 kB
./dist/assets/js/33-********************.js 3.72 kB
./dist/assets/js/34-********************.js 51.9 kB
./dist/assets/js/35-********************.js 16.1 kB
./dist/assets/js/36-********************.js 70.9 kB
./dist/assets/js/37-********************.js 31.6 kB
./dist/assets/js/38-********************.js 3.12 kB
./dist/assets/js/analytics-advanced-tracking-********************.js 769 B
./dist/assets/js/googlesitekit-base-********************.js 1.13 kB
./dist/assets/js/googlesitekit-datastore-location-********************.js 2.08 kB
./dist/assets/js/googlesitekit-datastore-site-********************.js 14.9 kB
./dist/assets/js/googlesitekit-i18n-********************.js 3.92 kB
./dist/assets/js/googlesitekit-idea-hub-notice-********************.js 45.1 kB
./dist/assets/js/googlesitekit-modules-********************.js 19.5 kB
./dist/assets/js/runtime-********************.js 1.34 kB

compressed-size-action

github-actions[bot] avatar Aug 19 '22 14:08 github-actions[bot]

Additionally, the padding/margins seem off, there's more padding once the "Set up now" button is clicked.

This existed in develop and has been fixed by #5276. I've merged develop into this PR branch and this issue is resolved now.

I tried the QA steps but the "Create Property" button doesn't seem to show the spinner:

I wasn't able to make it show up in my local setup normally as well, so I thought it must be something wrong on my end. In my local setup, both getPropertyID() and getWebDataStreamID() selectors of the modules/analytics-4 store return an empty string. So I basically just altered this condition and the spinner showed up. Do you have any idea how I could remedy this?

Thank you, @tofumatt!

nfmohit avatar Aug 24 '22 22:08 nfmohit

Thank you so much for pointing that out, @tofumatt!

In the screen where the user gets to select a property (i.e. a measurement ID has been created and is available), I believe we want to show a CTA there that says Connect (Figma), clicking on which should dispatch the submitChanges action of the modules/analytics-4 store. This functionality is being implemented in #5282: https://github.com/google/site-kit-wp/blob/ed7b8c9ef5b16c91ca2363d3dfd829f641ee6557/assets/js/modules/analytics-4/components/dashboard/ActivationBanner/SetupBanner.js#L55-L72

Do you agree that would cover it? I can surely go ahead and add it in this PR, but that would be a little duplicate/similarily between both PRs.

I'm open to your and @techanvil's suggestions. Thank you!

nfmohit avatar Sep 01 '22 00:09 nfmohit

Thank you so much for clearing this, @tofumatt!

I did some more thorough reading and observation of the Design Docs and the Figma designs, and I think if there are no existing GA4 properties available and the user creates a new property, we shouldn't display the property select and directly take them to the SuccessBanner. @techanvil Could you help us confirm if that is correct?

Thank you!

nfmohit avatar Sep 02 '22 06:09 nfmohit

Thank you so much for clearing this, @tofumatt!

I did some more thorough reading and observation of the Design Docs and the Figma designs, and I think if there are no existing GA4 properties available and the user creates a new property, we shouldn't display the property select and directly take them to the SuccessBanner. @techanvil Could you help us confirm if that is correct?

Thank you!

Hi @nfmohit, @tofumatt - it's a fair question, but as Nahid has pointed out, in this case the intention is to click on "Create property" and then go directly to the Success banner on success.

It's more analogous to the Setup screen where selecting "Set up a new property" and clicking "Configure Analytics" will go straight through to the Congrats banner without showing the new property in the dropdown.

https://user-images.githubusercontent.com/18395600/188103815-6b7be390-847d-43b3-bc39-e18b083ef01b.mp4

techanvil avatar Sep 02 '22 09:09 techanvil

Thank you for the confirmation, @techanvil!

I have updated the PR to go to the SuccessBanner after a successful submission.

nfmohit avatar Sep 02 '22 10:09 nfmohit

Looks good @nfmohit, I just have one small suggestion and this should be good to go.

I've updated it, thank you @techanvil!

nfmohit avatar Sep 02 '22 18:09 nfmohit

Hi @nfmohit, apologies as I had thought this PR was good to go. However I subsequently realised the case where an existing property does exist also needs to be covered (see https://github.com/google/site-kit-wp/issues/5277#issuecomment-1236879040).

In order to try to progress this issue today I merged develop into the ticket and resolved merge conflicts. However, having done this one small aspect does stand out. I'm no longer sure this check for ! ga4MeasurementID is necessary - don't we always want to render the CTA button on this component?

https://github.com/google/site-kit-wp/blob/551ae6841f95de55d6437a533b35176a80d528b7/assets/js/modules/analytics-4/components/dashboard/ActivationBanner/SetupBanner.js#L203-L204

Secondly, please can you verify the GA4 Property Select dropdown and "Connect" button work as expected in the "existing property" flow, i.e. it correctly connects the selected GA4 component, creating it as necessary? I think it should do but if not please can you make whatever change is needed to make this use case work too. With this working, please can you also update the QAB to include the "existing property" flow?

techanvil avatar Sep 05 '22 17:09 techanvil

I've addressed the suggestions. Thank you, @techanvil!

nfmohit avatar Sep 06 '22 10:09 nfmohit