Improve new Gathering Data Notifications to include link to add more services

wpdarren commented 2 years ago

Bug Description

While testing #4698 I noticed an improvement on the new dashboard notification messages for gathering data. We are asking users to think about connecting more services. It would be more user friendly to add a link to the connect more services tab within Site Kit settings. (wp-admin/admin.php?page=googlesitekit-settings#/connect-more-services)

We have done this with the new service connect notifications with serviceSetupV2

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

Acceptance criteria

Add a primary CTA in the Gathering Data Notification that links to the "Connect More Services" tab in Site Kit Settings. The CTA should be labeled "See other services".
The secondary CTA should now be the standard "Maybe later" link used in other notifications in the plugin which dismisses the notification in the same way that "OK, Got it!" did before for this notification.

Implementation Brief

Note: The IB below is implemented in a quick POC branch here.

In assets/js/components/notifications/ZeroDataStateNotifications/GatheringDataNotification.js:
- Add ctaLabel as per the AC.
- Set the CTALink using the getAdminURL( 'googlesitekit-settings' ) method from the core/site store and concatenate #/connect-more-services to it.

Fixing an issue where clicking on the CTA Link does not persist the dismissal

Within /assets/js/components/notifications/BannerNotification/index.js:
- Currently, handleCTAClick() calls dismissNotification(). However, within dismissNotification the code wrapped inside of setTimeout() is not executed because the default event of clicking on an href link and navigating to it happens before setTimeout() is reached.
- To fix this, we need to call event.preventDefault() higher up in the handleCTAClick function if there is a CTA link. Then we need to ensure we await the actions that happen within dismissNotification's setTimeout by returning a new promise and awaiting its resolution. And then we can proceed to navigateTo( ctaLink ).
- The above fix introduces a flicker where the component is disappearing and appearing again before navigating to a new page. This happens because dismissNotification closes the notification by setting setIsClosed( true ). If we wrap this call in a condition where this should be skipped when a ctaLink exists, then this flicker is resolved.

Test Coverage

Add a new JS test in BannerNotifications.test.js to test that clicking on a CTA link when the isDissmissible prop is true dismisses the notification.

QA Brief

Ensure Search Console or Analytics is in gathering data state. This can be done using the tester plugin or creating a new GA4 property.
Verify the See other services primary CTA in the gathering data state banner notification.
Ensure clicking the primary CTA navigates to the "Connect More Services" tab in Site Kit Settings.
Ensure the banner notification is dismissed.
Ensure the secondary CTA is renamed "Maybe later" and dismisses the banner notification.

Changelog entry

Improve the notification banner that is shown when Site Kit is gathering data.

marrrmarrr commented 2 years ago

At this stage, the users should see CTAs within the dashboard widgets to activate GA and AdSense (if they haven't already activated GA during setup). PSI is already activated, so the only thing they could really activate would be Optimize or GTM (no effect on reporting within the dashboard). @felixarntz what are your thoughts on this, should we funnel people to Settings?

felixarntz commented 2 years ago

@marrrmarrr Yeah, I think including a link to Settings > Connect more services there would make sense. Does that sound good to you?

jimmymadon commented 11 months ago

@marrrmarrr With the sunset of Optimize and potentially removing support for GTM in the future (based on the Singular Analytics Module Design doc), there aren't many services left to connect once SC and Analytics are connected. So instead of creating a "Primary CTA button", I feel a secondary link to the Connect More Services tab in Site Kit settings is better. Since this is a simple notification, I don't see too much value in complicating the logic here and checking for "which modules are connected?" before slightly changing the notification message / CTA.

c.c. @aaemnnosttv

UPDATE: The only other alternative here is to close this issue or actually modify it so that we remove the second sentence from the notification.

techanvil commented 11 months ago

Hey @jimmymadon, it would be good to specify how the link should look here. As it stands, I would take it to mean we make the phrase "connect more services" a link. However, reviewing the setup success notification which is referred to in the Bug Description, I see it has a separate "Go to Settings" link. Presumably for consistency we should do the same here. WDYT?

marrrmarrr commented 11 months ago

If the primary action here is to connect more services, then that should be the primary CTA as well. So I'd suggest "Let's go" or something to that effect that would send people to settings and "Maybe later" as the standard secondary CTA if they don't want to connect more things right away.

@jimmymadon if the person hasn't connected AdSense at this stage, wouldn't there be a tile for that as well in Settings? Separately, we'll be adding AdWords soon, so there will be more things to connect, if the user chooses to do so.

More generally, I think we need to rethink the messages we send after setup is complete, to provide a smoother onboarding experience (e.g. instead of directing people to connect more stuff, do a tour of the dashboard and what's on there). But that's a discussion for another GH issue (:

techanvil commented 11 months ago

AC :white_check_mark:

Note that I made a minor amendment to change "link" to "CTA" in the AC.

tofumatt commented 11 months ago

IB ✅

mxbclang commented 11 months ago

@tofumatt @jimmymadon This issue is in EB but doesn't have an estimate – can you please add? Thanks!

jimmymadon commented 11 months ago

@bethanylang Oops - just added one.

hussain-t commented 10 months ago

Hi @marrrmarrr, do we need to dispatch an event when clicking the primary CTA? If so, can we use the existing dismiss_notification or a new one?

aaemnnosttv commented 10 months ago

@hussain-t primary CTAs use confirm_notification, dismiss would only be for the "maybe later" case. Since the notification already implements these other common events, it should do the same for the primary CTA as well.

hussain-t commented 10 months ago

Thanks, @aaemnnosttv 👍

mohitwp commented 10 months ago

QA Update ⚠️

Tested on dev environment.
Verified for both cases 1) when only SC is connected and in gathering state. 2) When both analytics and search console are connected and in gathering state.
Verified that clicking on "See other services" navigates to the "Connect More Services" tab in Site Kit Settings.
Verified that clicking on "Maybe later" CTA dismissed the banner.

@hussain-t I have one question that clicking on "See more Services" or "Maybe later" CTA dismisses the Banner which appears again when we clear session data or admin login again. It is same as current functionality on latest. Can you please confirm that it is correct and we will not dismiss the banner permanently ?

https://github.com/google/site-kit-wp/assets/94359491/5d9ca315-886a-4e5c-ae16-96051c069f6e https://github.com/google/site-kit-wp/assets/94359491/b06613fa-67f5-4d40-aaff-92962eb3fb21 ![image](https://github.com/google/site-kit-wp/assets/94359491/f6bd37d5-2a44-4bfe-8a13-1224c4c7c9af)

hussain-t commented 10 months ago

I have one question that clicking on "See more Services" or "Maybe later" CTA dismisses the Banner which appears again when we clear session data or admin login again. It is same as current functionality on latest. Can you please confirm that it is correct and we will not dismiss the banner permanently ?

@mohitwp, that's expected since we don't permanently dismiss the gathering / zero data notifications.

mohitwp commented 10 months ago

Thanks @hussain-t !

QA Update ✅

Tested on dev environment.
Verified for both cases 1) when only SC is connected and in gathering state. 2) When both analytics and search console are connected and in gathering state.
Verified that clicking on "See other services" navigates to the "Connect More Services" tab in Site Kit Settings.
Verified that clicking on "Maybe later" CTA dismissed the banner.

https://github.com/google/site-kit-wp/assets/94359491/5d9ca315-886a-4e5c-ae16-96051c069f6e https://github.com/google/site-kit-wp/assets/94359491/b06613fa-67f5-4d40-aaff-92962eb3fb21 ![image](https://github.com/google/site-kit-wp/assets/94359491/f6bd37d5-2a44-4bfe-8a13-1224c4c7c9af)

google / site-kit-wp