Extract "Subtle Notification" styling/logic to a shared SubtleNotification component

[tofumatt]

Feature Description

We intentionally did not create a "Subtle Notification" component for the new styles of component rendered by components like https://github.com/google/site-kit-wp/blob/f5a89cf2cd5978ee012308281a2840e03192b2c4/assets/js/components/notifications/GA4AdSenseLinkedNotification.js

But now that we have a few of these "Subtle Notifications", we can see how they're used in real-world scenarios and should be able to extract the logic for them into a common, shared component (eg. SubtleNotification) that all existing notifications should move to using and new notifications can use.

---

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

Acceptance criteria

• A new component for subtle notifications, eg. SubtleNotification should be created that implements the subtle notification components, with the ability to provide a header, text, and CTA actions, eg:

• Existing "subtle notification" components, eg. GA4AdSenseLinkedNotification, SetupSuccessSubtleNotification, and AudienceSegmentationSetupSuccessSubtleNotification, should be used to extract this new component. They should then be converted to use <SubtleNotification> instead of the existing approach.

Implementation Brief

• Create a new component assets/js/modules/analytics-4/components/SubtleNotification.js, that has the following props:
  • title (string): Title passed through the __ function.
  • description (ReactNode): Description passed as an element to enable DOM elements such as links in the description in future.
  • onDismiss (function): Function to call to dismiss the notification.
  • additionalCTA (ReactNode|false): An optional CTA component (used for assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification.js to show the "Show me" CTA)
  • Copy the DOM from the return statement of assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification.js, replacing the title, description, dismiss function and additional CTA components with the values passed from the props.
• Update each existing subtle notification, <AudienceSegmentationSetupSuccessSubtleNotification />, <GA4AdSenseLinkedNotification />, <SetupSuccessSubtleNotification /> and <PAXSetupSuccessSubtleNotification /> to use the new assets/js/modules/analytics-4/components/SubtleNotification.js instead of duplicating the DOM.
• Keep the render logic in each existing notification component as this logic varies between each of these components.

Test Coverage

• Create a new SubtleBanner story and VRT.

QA Brief

• Set up Site Kit with an Analytics and AdSense account that are linked.
  • Ensure the notification that your Analytics and AdSense accounts are linked still appears after setting up AdSense.
• Set up Ads and add a Conversion Tracking ID to the Ads module settings.
  • Ensure the notification that says "Success! Your Conversion Tracking ID was added to your site" appears

Changelog entry

• Implement a shared SubtleNotification component and update existing notifications to use it.

[benbowler]

@tofumatt here's my first pass on the IB. We could simplify this further by also bringing the dismiss logic into the new SubtleNotification component and give it a slug. Assuming we're happy to unify event tracking.

---

Taking it further we could bring the render logic into the component also, but I don't see much of a benefit here as some notification render logic is more complex than others so we'd need to pass it as a boolean prop from the parent componet. I believe it's cleaner to have this logic in a component within the module it relates to and to return null here instead of rendering the component.

site-kit-wp/assets/js/components/notifications/GA4AdSenseLinkedNotification.js

Lines 168 to 182 in 7704dc4

	// Ensure resolution of the report has completed before showing this
	// notification, since it should only appear when the user has no data in
	// the report.
	const shouldShowNotification =
	// Only show this notification on the main/entity dashboard, not on the
	// settings page, etc.
	dashboardType &&
	// Don't show this notification on the view-only dashboard.
	! viewOnly &&
	// Don't show this notification if `isDismissed` call is still loading
	// or the user has dismissed it.
	! isDismissed &&
	hasFinishedResolution &&
	isZeroReport( report ) &&
	analyticsAndAdsenseConnectedAndLinked;

[tofumatt]

@benbowler To future-proof this one a bit, I think at least the description prop should expect a React Node rather than a string. It's minor, but probably better to specify that now. I could imagine a world where there's a "Learn more" link or "Get help" link in there alongside 1-2 CTA buttons (that tend to dismiss the notification), so better to budget for at least a more flexible description prop now.

I agree about keeping the logic to display the notification inside each component rather than it being passed as a prop. It's more awkward that way 👍🏻

---

Re: your idea about unifying the dismiss logic: I think it seems to make sense to consolidate that dismissal into the component, but in reality it's just one line. 🤔

site-kit-wp/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification.js

Line 64 in b820762

dismissItem( AUDIENCE_SEGMENTATION_SETUP_SUCCESS_NOTIFICATION );

Given the components themselves will already be checking that slug to see if it is dismissed to know whether they should render the notification, it probably makes sense to just let each component render as many CTA buttons at they like.

Plus, the logic to dismiss each notification isn't even totally consistent in the plugin right now. The PAX one just unsets a query arg:

site-kit-wp/assets/js/modules/ads/components/notifications/PAXSetupSuccessSubtleNotification.js

Line 41 in b820762

setNotification( undefined );

I think passing onDismiss and then the: additionalCTA as an extra prop makes sense. Given it just needs to be a <Button onClick={}>Text</Button> component, as the notification itself styles the button to look correct in there, I think that's the smart approach, nice 👍🏻

---

If you're cool with description being a React Node instead of string I'm cool with this approach, it's simple and easy-to-use 🙂

[benbowler]

Thanks @tofumatt updated.

[tofumatt]

Awesome, thanks!

IB ✅

[techanvil]

Hey @10upsimon, just want to give you a heads up that I'm making a couple of changes to the SCSS for the subtle notifications in PR #9046. Looking at the IB for this issue, it might not actually affect it, but I figure it's worth a mention just in case!

[wpdarren]

@mohitwp for reference. I have chatted to @tofumatt about this and he's going to update the QAB. We should complete manual testing on the subtle notifications in the AC plus other subtle notifications like when you set up Ads module, etc.

[tofumatt]

QA brief updated 🙂

[mohitwp]

QA Update ⚠️

• Tested on dev environment.
• Verified the Ads module subtle notification.
• Verified the Audience segmentation subtle notification.

@tofumatt According to the QAB, we need to ensure that the notification about linking your Analytics and AdSense accounts still appears after setting up AdSense. However, this notification no longer displays in the latest and dev environments because we now show data immediately if the AdSense and Analytics accounts are linked. Previously, the notification appeared because we didn't show data right away. In case of gathering and zero state also subtle notification not appears.
Please let me know if this is not the expected behavior, and I will create a separate ticket for this issue. cc @wpdarren

Recording.1244.mp4

Pass Cases

[tofumatt]

@mohitwp Oh, right, I forgot about that, and I had tested it another (more involved) way 😅

That's totally fine, I think the pass cases are enough to move this to approval, so I've moved it there 👍🏻

[mohitwp]

QA Update ✅

Thanks @tofumatt !

• Tested on dev environment.
• Verified the Ads module subtle notification.
• Verified the Audience segmentation subtle notification.
• Verified that the subtle notifications are appearing for both Ads and Audience segmentation.