google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 284 forks source link

Show a small notification when AdSense + GA4 accounts are detected as linked #8238

Closed tofumatt closed 6 months ago

tofumatt commented 7 months ago

Feature Description

When AdSense + GA4 are detected as linked, a small notification should be shown (see Figma design and screenshot below) to alert users that their accounts are now successfully detected as linked.

Note that this is different from the overlay shown in #8236, which should only happen once we have actually detected data from linking the two accounts in the "Top Earning Pages" widget in the Monetization section of the dashboard.

This notification should be dismissed automatically when the notification from #8236 is dismissed/interacted with.

Figma design: https://www.figma.com/file/7ba0pj1rLuvLvJhy3NiHOj/AdSense?type=design&node-id=10-7440&mode=design&t=c0U4LmJSFQ5Fe9ry-0

Screenshot 2024-02-19 at 23 30 50


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

Acceptance criteria

Implementation Brief

SubtleNotifications

GA4AdSenseLinkedNotification

Test Coverage

QA Brief

Changelog entry

mxbclang commented 7 months ago

@tofumatt @marrrmarrr Same comment as the other two, should we add a "The" in front of "top earning content"?

marrrmarrr commented 7 months ago

@tofumatt @bethanylang I've added copy in the ACs and added a note about triggering.

@tofumatt based on the comments in the issue description, it seems like there might be a period where the widget is appearing, but there's no data to show, is that right? If that's the case, I'll need to adjust the copy a bit more.

To clarify how this was intended in terms of user flows (we didn't factor in the possibility of no data for a while):
1) This CTA (also described in #8236) should be shown to users who have AdSense and GA4, but have not linked them. 2) The success notice from this GH issue is meant purely as a follow up for this flow -- show to users who linked AdSense and GA4 as a result of the previous CTA. 3) This CTA (also described in #8237) is meant to be shown to users who had previously linked their GA4 and AdSense accounts, just to alert them that the feature is back (as no action is required on their end).

Showing both the CTA from #8237 and the success notice from #8238 to the same user would be a bit of a visual overload IMO.

tofumatt commented 7 months ago

Showing both the CTA from https://github.com/google/site-kit-wp/issues/8237 and the success notice from https://github.com/google/site-kit-wp/issues/8238 to the same user would be a bit of a visual overload IMO.

Agreed. I've added to the ACs here that the success notice should not appear if the overlay in #8237 should appear. 👍🏻

@tofumatt based on the comments in the issue description, it seems like there might be a period where the widget is appearing, but there's no data to show, is that right? If that's the case, I'll need to adjust the copy a bit more.

Indeed, that will be the case for this notification. This would appear when we detect the accounts are linked, but don't have any data yet—it can take (I think) 1-2 days after linking before data is available in the report.

I'll leave this assigned to you to update the copy—I think we might just want a single CTA that says "Got it" because scrolling the user to the widget while it's in a "gathering data" state is not very useful. So a notice more like "Your AdSense and Analytics accounts are linked. Top earning pages widget will be available in a few days." seems better-suited for this notification.

benbowler commented 7 months ago

Added an IB and estimate.

tofumatt commented 7 months ago

useEffect check if a report data has been returned for the top earning pages report options

Shouldn't this be done in a useSelector?

Also:

In a useEffect check if a report data has been returned for the top earning pages report options, if so, call dismissItem to dismiss this banner:

dismissItem will also trigger the trackEvent call, which we shouldn't do if it isn't a user event. If we're hiding the notification because it isn't needed, we should dismiss the notification in the user's settings without actually logging an Analytics event, since they didn't actually interact with the notification at all.


Instead of assets/js/components/notifications/DashboardNotifications.js, how about SubtleNotifications or similar? The "banner" prefix of the banner notifications is at least a bit self-explanatory, in that they are large, "banner-style" notifications. But "DashboardNotifications" doesn't really distinguish them by name alone. Since these are small, less bold notifications, something like SubtleNotifications or SmallNotifications might make it easier to tell them apart. 🤔

benbowler commented 7 months ago

Thanks @tofumatt:

For this one:

In a useEffect check if a report data has been returned for the top earning pages report options...

I was thinking something like this will be required for the conditional check:

    const { dismissItem } = useDispatch( CORE_USER );

    const reportOptions = {
        ...dates,
        dimensions: [ 'pagePath' ],
        metrics: [ { name: 'totalAdRevenue' } ],
        orderby: [
            {
                metric: { metricName: 'totalAdRevenue' },
                desc: true,
            },
        ],
        limit: 3,
    };

    const report = useInViewSelect( ( select ) => {
        return select( MODULES_ANALYTICS_4 ).getReport( reportOptions );
    } );

    useEffect( () => {
        if ( report !== undefined ) {
            dismissItem(
                GA4_ADSENSE_LINKED_NOTIFICATION_DISMISSED_ITEM_KEY
            );
        }
    }, [ report, dismissItem ] );

I've updated this in the IB as well.


I've updated the the CTA onClick action to the following:

When the CTA is clicked it should dismiss the notification by calling dismissItem directly.


Agreed about the notification section name, I had a few thoughts of how best to name this section, I've updated it to go with SubtleNotifications, I like the sound of that.

tofumatt commented 7 months ago

IB ✅

mohitwp commented 6 months ago

QA Update ❌

@zutigrm Seems like Figma link is not correct. Can you please check ? Also, find my observations below.

Issue : I noticed that in mobile viewports having width 599px and less. Notification content is appearing without icon and Primary CTA Got it appearing in center. I think it will look good if it is left align.

Note: Also, I will not able to test one point mentioned in AC because related PR https://github.com/google/site-kit-wp/issues/8237 is not merged yet.

![image](https://github.com/google/site-kit-wp/assets/94359491/5ec43cc3-3f35-4a89-94a5-54aa69e94097) ![image](https://github.com/google/site-kit-wp/assets/94359491/d8b9902f-70b1-47bb-91db-cfc4718edf4e)
zutigrm commented 6 months ago

Hi @mohitwp thanks for the observations

I noticed that in mobile viewports having width 599px and less. Notification content is appearing without icon and Primary CTA Got it appearing in center

Yes, this is intended, icon is hidden as suggestion during CR process. Note that there is no mobile design for this layout, only desktop. I can align it to the left on mobile

Figma link is leading to the correct file, it is under iteration 1 tab. I linked directly to the notification here - https://www.figma.com/file/7ba0pj1rLuvLvJhy3NiHOj/AdSense?type=design&node-id=100-2626&mode=design&t=eEF24a639vMrryCW-0

UPDATE: Only the spacing on screens bellow 599px is edited. We agreed to keep button centered (as is)

mohitwp commented 6 months ago

QA Update ❌

Issue > Clicking on primary CTA 'Got it' is not dismissing the notification. Nothing happens when user clicks on primary CTA.

image

Pass Cases

![image](https://github.com/google/site-kit-wp/assets/94359491/eba1f3e7-3536-4fcc-bdbd-6d27cfc6fa7a) ![image](https://github.com/google/site-kit-wp/assets/94359491/ebc56d8e-272c-4dc3-8ddc-9ba384e66cf4) ![image](https://github.com/google/site-kit-wp/assets/94359491/fd96c4cd-41a3-41bf-9c85-e4d6de814c9b)
kuasha420 commented 6 months ago

@mohitwp Back to you for another pass. Please test on main. Cheers.

mohitwp commented 6 months ago

QA Update ✅

https://github.com/google/site-kit-wp/assets/94359491/d65edf1b-14d6-4610-b0f6-576afd332563 ![image](https://github.com/google/site-kit-wp/assets/94359491/eba1f3e7-3536-4fcc-bdbd-6d27cfc6fa7a) ![image](https://github.com/google/site-kit-wp/assets/94359491/ebc56d8e-272c-4dc3-8ddc-9ba384e66cf4) ![image](https://github.com/google/site-kit-wp/assets/94359491/fd96c4cd-41a3-41bf-9c85-e4d6de814c9b)