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

Refactor `GoogleTagIDMismatchNotification` to use the new Notifications datastore

Open zutigrm opened this issue 1 year ago • 1 comments

Feature Description

This issue should refactor GoogleTagIDMismatchNotification so that it uses the new datastore infrastructure to register and queue the notification. It should also incorporate a new "layout"


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

Acceptance criteria

  • The GoogleTagIDMismatchNotification component should be refactored so that it is registered and rendered (queued) using the new core/notifications datastore.
  • This notification component should not be called directly (i.e. in BannerNotifications) but only via the generic getQueuedNotifications selector.
  • The refactored component should not contain any business logic that hides it / prevents it from rendering. This logic should be contained in a callback function defined during registration.
  • The component should not use the bloated BannerNotification component. Instead, it should be rendered using the new Notification component wrapper and a simpler "layout" component that solely encapsulates its structure and design.

Implementation Brief

  • [ ] Update assets/js/components/notifications/GoogleTagIDMismatchNotification.js:

    • The component should except new props { id, Notification }.
    • Combine the rendering of both variants of the component, wrapping the new render with Notification component from the component props.
      • https://github.com/google/site-kit-wp/blob/cc981876c3b654437e965cb66f8a09c9d110dba1/assets/js/components/notifications/GoogleTagIDMismatchNotification.js#L167-L236
      • Use the existing layout assets/js/googlesitekit/notifications/components/layout/NotificationError.js.
        • Conditionally pass the title, description and action props based on the values of newAnalyticsProperty and newGoogleTagID.
        • Update NotificationError, adding a new prop hideIcon, if true hide the BannerIcon and pass true in this component.
  • [ ] Update assets/js/googlesitekit/notifications/register-defaults.js:

    • checkRequirements should use this rendering logic:
    • https://github.com/google/site-kit-wp/blob/cc981876c3b654437e965cb66f8a09c9d110dba1/assets/js/components/notifications/BannerNotifications.js#L72-L74
    • isDismissible should be false.
    • For priority, should be 150 as this is an error.
    • Use NOTIFICATION_AREAS.BANNERS_ABOVE_NAV for areaSlug.
  • [ ] Remove all references to GoogleTagIDMismatchNotification in assets/js/components/notifications/BannerNotifications.js.

Test Coverage

  • Update the stories in assets/js/components/notifications/GoogleTagIDMismatchNotification.stories.js confirm both variants render correctly based on the existing render logic in the new notification infrastructure.

QA Brief

Changelog entry

zutigrm avatar Sep 04 '24 12:09 zutigrm

AC ✔️

eugene-manuilov avatar Sep 06 '24 17:09 eugene-manuilov

  • Update assets/js/googlesitekit/notifications/register-defaults.js:

@benbowler, let's move the GoogleTagIDMismatchNotification notification to be defined and registered in the Analytics module since this is the module that notification belongs to.

eugene-manuilov avatar Dec 02 '24 19:12 eugene-manuilov

  • Update assets/js/components/notifications/GoogleTagIDMismatchNotification.js:

@benbowler, this component should be moved to the Analytics module.

eugene-manuilov avatar Dec 03 '24 13:12 eugene-manuilov

IB ✔

eugene-manuilov avatar Dec 04 '24 09:12 eugene-manuilov

QA Update ⚠

  • Tested on dev environment.
  • Verified GoogleTagIDMismatchNotification components.
  • Verified both Banner A and Banner B of Google tag ID mismatch notifications are showing.
  • Verified both Banner A and Banner B have correct copy.
  • Verified new Storybook Scenarios created under GoogleTagMismatchIdNotification.
  • Verified all other notifications on the site especially those that have both, a Primary (CTA Button) and Secondary (CTA Link).
  • Verified that new GA events for each CTA action similar to other notification refactoring issues - view_notification, confirm_notification, and dismiss_notification.
  • Note: I found an issue related to AdSense—'Earning at Risk' notification appears when setting up Tag Manager. However, this issue also exists in the latest environment, so I will create a separate ticket for it.

QUESTION @jimmymadon, I noticed that after reloading twice, Banner A appears, but it takes 5-6 seconds( On 5G network) to display on the main dashboard, whereas Banner B appears immediately.

Is there a way to make Banner A display faster, similar to Banner B?

Image

https://github.com/user-attachments/assets/b1414d0c-583c-44f0-95ff-cc430b062b35

Google ID mismatch Notifications

Banner A

Image

Banner Scenario B

Image

https://github.com/user-attachments/assets/67819825-3343-4cbd-a0d0-ae6a4459cabd

https://github.com/user-attachments/assets/6b936746-cca0-44b4-9146-f3f121201157

All other notifications

Image

Image

Image

Image

Image

Image

Image

Image

Image

Image

Image

Image

Storybook Stories

Image

Image

mohitwp avatar Mar 19 '25 00:03 mohitwp

@mohitwp I've tested this but unfortunately, there isn't much we can do immediately here to speed this up. Even the legacy notice took quite a while to load - just that it happened without an additional page reload. We are planning to revamp the way these time consuming banners load on the main dashboard in the upcoming Notification Centre epic.

I will raise this in my 1:1 with @aaemnnosttv later today. Considering this isn't a regression and this banner itself is quite "rare", we can let this move forward and tackle slower loading notifications separately.

jimmymadon avatar Mar 19 '25 09:03 jimmymadon

QA Update ✅

Thanks @jimmymadon ! I will create a separate ticket for this.

  • Tested on dev environment.
  • Verified GoogleTagIDMismatchNotification components.
  • Verified both Banner A and Banner B of Google tag ID mismatch notifications are showing.
  • Verified both Banner A and Banner B have correct copy.
  • Verified new Storybook Scenarios created under GoogleTagMismatchIdNotification.
  • Verified all other notifications on the site especially those that have both, a Primary (CTA Button) and Secondary (CTA Link).
  • Verified that new GA events for each CTA action similar to other notification refactoring issues - view_notification, confirm_notification, and dismiss_notification.
  • Note: I found an issue related to AdSense—'Earning at Risk' notification appears when setting up Tag Manager. However, this issue also exists in the latest environment, so I will create a separate ticket for it.

Google ID mismatch Notifications

Banner A

Image

Banner Scenario B

Image

https://github.com/user-attachments/assets/67819825-3343-4cbd-a0d0-ae6a4459cabd

https://github.com/user-attachments/assets/6b936746-cca0-44b4-9146-f3f121201157

All other notifications

Image

Image

Image

Image

Image

Image

Image

Image

Image

Image

Image

Image

Storybook Stories

Image

Image

mohitwp avatar Mar 19 '25 13:03 mohitwp

Update : I created separate issues for my observations reported above - #10488 and #10489

mohitwp avatar Mar 20 '25 08:03 mohitwp