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

Scaffold the Setup Component variants #5275

Closed
techanvil opened this issue May 25, 2022 · 10 comments
Closed

Scaffold the Setup Component variants #5275

techanvil opened this issue May 25, 2022 · 10 comments
Labels
P1 Medium priority Type: Feature New feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented May 25, 2022

Feature Description

Prepare the Setup Component variants based on the presence of an existing GA4 property in the user's Analytics account ("existing GA4 property") and/or existing manually inserted tag on the page ("existing tag").

  • No existing GA4 property, no existing tag.
    • CTA button: Create property.
  • Existing GA4 property, no existing tag.
    • CTA button: Connect.
  • Existing GA4 property, existing tag.
    • CTA button: Connect.
  • No existing GA4 property, existing tag.
    • CTA button: Create property.

Ensure the relevant variant is displayed, showing the text as per the design. If necessary, modify the BannerNotification component to allow content to be displayed below the CTA - a footer prop could be appropriate.

However do not implement the dropdown or toggle controls for the "existing GA4 property" variants, they will be added separately.


Acceptance criteria

  • The placeholder version of the Setup Component (see Scaffold GA4 Activation Banner "happy path" UI #5270) should be replaced with the fully-realised design - except for the dropdown and toggle controls (which will be implemented in another issue, see: Add the dropdown and toggle controls to "existing GA4 property" setup variants #5276). For reference, see Figma.
  • For each variant display text as follows.
    • No existing GA4 property, no existing tag.
      • Title: No existing Google Analytics 4 property found, Site Kit will help you create a new one and insert it on your site
      • CTA button: Create property.
      • Footer: You can always add/edit this in the Site Kit Settings.
    • Existing GA4 property, no existing tag.
      • Title: Connect the Google Analytics 4 property that’s associated with your existing Universal Analytics property
      • CTA button: Connect.
      • Footer: You can always add/edit this in the Site Kit Settings.
    • Existing GA4 property, existing tag.
      • Title: Connect the Google Analytics 4 property that’s associated with your existing Universal Analytics property
      • CTA button: Connect.
      • Footer: You can always add/edit this in the Site Kit Settings.
    • No existing GA4 property, existing tag.
      • Title: No existing Google Analytics 4 property found, Site Kit will help you create a new one and insert it on your site
      • CTA button: Create property.
      • Footer: A GA4 tag {property_ID} is found on this site but this property is not associated with your Google Analytics account. You can always add/edit this in the Site Kit Settings.

Implementation Brief

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

  • Add an optional footer prop of type PropTypes.node.
  • If footer is provided, render it within a new Row at the end of the Grid.

In assets/js/modules/analytics-4/components/dashboard/ActivationBanner/SetupBanner.js:

  • To check for an existing GA4 property, use the MODULES_ANALYTICS_4 store's getMeasurementID selector.
  • To check for an existing tag, use the MODULES_ANALYTICS_4 store's getExistingTag selector.
  • For each variant described in the AC, set the corresponding BannerNotification props:
    • title with the specified Title text.
    • ctaLabel with the specified CTA button text.
    • footer with the specified Footer text.
  • The footer may benefit from wrapping in a paragraph p for styling.

Make any styling updates needed.

  • BannerNotification styles can be found in assets/sass/components/global/_googlesitekit-publisher-wins.scss
  • Consider adding assets/sass/components/activation/_googlesitekit-ga4-activation-banner.scss if it makes sense to do so.

Test Coverage

  • Add Storybook stories for the new Activation Banner variations and add to the VRT suite.

QA Brief

  • Activate the ga4ActivationBanner feature flag from the tester plugin.
  • Go to the Site Kit Dashboard.
  • Click on Set up now in the displayed banner.
  • Verify that the banner displayed afterwards matches the text in the AC and designs in Figma (do not that due to the GM2+, some styles may be different).
  • Verify the Modules/Analytics4/ActivationBanner/SetupBanner story displays correctly.

Changelog entry

  • N/A
@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 21, 2022
@hussain-t hussain-t removed their assignment Jul 25, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Jul 28, 2022
@eugene-manuilov
Copy link
Collaborator

In assets/js/modules/thank-with-google/components/dashboard/ActivationBanner/SetupBanner.js:

@techanvil I believe this file should belong to the analytics-4 module, shouldn't it?

@techanvil
Copy link
Collaborator Author

In assets/js/modules/thank-with-google/components/dashboard/ActivationBanner/SetupBanner.js:

@techanvil I believe this file should belong to the analytics-4 module, shouldn't it?

Whooops, that was a copy/paste error. I've fixed it - thanks for spotting!

@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Aug 11, 2022
@nfmohit nfmohit assigned eugene-manuilov and unassigned nfmohit Aug 11, 2022
@nfmohit nfmohit assigned eugene-manuilov and unassigned nfmohit Aug 12, 2022
@eugene-manuilov eugene-manuilov removed their assignment Aug 12, 2022
@wpdarren wpdarren self-assigned this Aug 15, 2022
@wpdarren
Copy link
Collaborator

QA Update: ❌

@nfmohit I have a few observations:

  1. No existing Google Analytics 4 property found, Site Kit will help you create a new one and insert it on your site The font size of the title is 20px on figma design but 28px on my test site.

  2. You can always add/edit this in the Site Kit Settings. The font size of the text is 16px on the figma but 14px on my test site.

This applies to both of the screens: 1) Set up Google Analytics 4 now to join the future of Analytics and 2) No existing Google Analytics 4 property found, Site Kit will help you create a new one and insert it on your site

  1. The alignment of the notification does not appear to be correct. If you look at the figma design, the title, description and CTA button are aligned to the Google Site Kit icon. On my test site it's not. The notification appears to have more padding/margin around it too. Compare it with the first screen Set up Google Analytics 4 now to join the future of Analytics and you'll see the differences.

Figma design
image

Test site
image

@wpdarren wpdarren assigned nfmohit and unassigned wpdarren Aug 15, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Aug 17, 2022

Thank you for your findings, @wpdarren!

For issues 1 & 2, the implemented font sizes reflect the GM2+ styles in order to keep them consistent with the other Banner Notifications. However, for the footer text, even though it is newly introduced in this task, I decided to keep it consistent with the existing paragraph font size, so that it doesn't look bigger than the description text in any other case.

For issue 3, you're right. I'm currently working on #5276 and have included a fix for it there. Would that work?

CC: @techanvil to validate my points, thank you!

@techanvil
Copy link
Collaborator Author

Hey @nfmohit, thanks for the tag.

  1. Actually, I do think we need to reduce the font size for the title here. Notice that in Figma, the font size for the title on the Reminder component is larger than the Setup component:

image

image

As it happens the font sizes in Figma are 28px for the Reminder vs 20px for Setup, so I think 20px would be the right value to use in this case.

  1. Your reasoning makes sense for the footer text, we want to be consistent with other Banner Notifications, which have description text with 14px font size. Keeping the footer text the same size as the description text would in fact be consistent with the Figma design. So 14px does look like the correct size here.

  2. You may want to reassess this considering point 1 also needs a fix... Maybe a followup PR would be more appropriate but if you prefer to roll it into Add the dropdown and toggle controls to "existing GA4 property" setup variants #5276 and @wpdarren is happy with that too, that sounds perfectly OK to me. If you do take the followup PR route feel free to ping me for a review.

@wpdarren
Copy link
Collaborator

@nfmohit @techanvil I am happy to go with whatever is easier for you. I can include these as known issues that will be fixed in 5276.

@nfmohit
Copy link
Collaborator

nfmohit commented Aug 18, 2022

Thank you for the review, @techanvil!

@wpdarren That would be fab! I'm already working on #5276 and I have already included the fixes there. Thank you!

@wpdarren
Copy link
Collaborator

wpdarren commented Aug 19, 2022

QA Update: ✅

Verified:

  • The banner displayed matches the texts in the AC for the different scenarios.
Screenshots

image
image
image
image

Note: there are some differences in the FIgma designs which were highlighted in this ticket here. We decided that these will be fixed within 5276, so will check the styling when this is merged.

@wpdarren wpdarren removed their assignment Aug 19, 2022
@techanvil
Copy link
Collaborator Author

Adding a note to mention that the IB and subsequent implementation was a bit wrong here and we should have been checking the result of the analytics-4 getProperties selector to check for an existing property/properties, rather than getMeasurementID.

This has come up while QAing #5276 and a followup PR to that issue has been created to fix it: #5793

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

8 participants