search

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.22k stars 278 forks source link

Show a small overlay/callout to AdSense + GA4 users whose accounts are not linked #8236

Closed tofumatt closed 5 months ago

tofumatt commented 7 months ago

Feature Description

A new callout (styled somewhat similarly to our surveys in the bottom-right of the screen) should be added for users who have AdSense and GA4 connected, but whose accounts are not detected as "linked".

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

Screenshot 2024-02-19 at 23 33 11

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

Acceptance criteria

Implementation Brief

Showing only one overlay on-screen at a time

Because this pattern will be used for at least two notifications now (and likely will expand in the future), we should implement at least a basic system that prevents multiple overlays from appearing at the same time/on the same page-load.

Right now we can probably start simple, where only one notification can render itself at a time. It's probably fine to start by having every notification call its render function and whichever one determines it should render, dispatch the to the CORE_UI store to be the one the user sees for this page load.

In the future, we could add things like priority, etc., but this simple approach will work for now. A hook that allows us to check if a notification should be shown and allows us to set the active one (based on the first notification to meet all of its "should render" requirements) in assets/js/hooks/useOverlayNotification.js could look like:

import { useDispatch, useSelect } from '@wordpress/data';
import { useCallback } from '@wordpress/element';
import { CORE_UI } from '../googlesitekit/datastore/ui/constants';
import { useMemo } from 'react';

const OVERLAY_NOTIFICATION_TO_SHOW = 'overlayNotificationToShow';

export function useOverlayNotification( componentName ) {
    const { setValue } = useDispatch( CORE_UI );

    const notificationToShow = useSelect( ( select ) => {
        return select( CORE_UI ).getValue( OVERLAY_NOTIFICATION_TO_SHOW );
    } );

    const isShowingNotification = useMemo( () => {
        return notificationToShow === componentName;
    }, [ componentName, notificationToShow ] );

    const setOverlayNotificationToShow = useCallback( () => {
        // If we're already showing a notification, don't override it.
        if ( isShowingNotification ) {
            return;
        }

        setValue( OVERLAY_NOTIFICATION_TO_SHOW, componentName );
    }, [ componentName, isShowingNotification, setValue ] );

    return { isShowingNotification, setOverlayNotificationToShow };
}
function () {
  const { isShowingOverlayNotification, shouldShowOverlayNotification } = useOverlayNotification( 'LinkAnalyticsAndAdSenseAccountsOverlayNotification' );

  // (Could be wrapped in a useEffect, but as-is is safe to call multiple times.)
  shouldShowOverlayNotification( ga4AdSenseIntegrationEnabled && ! isViewOnly && analyticsConnected && adSenseConnected && adSenseLinked === true && linkAnalyticsAdSenseOverlayDismissed === false );

  if ( ! isShowingOverlayNotification ) {
    return null;
  }

  return <ComponentOutput />
}

There should be an OverlayRenderer component that outputs all overlays, which will allow the "active" one (if one exists) to be output, eg:

<Fragment>
  <LinkAnalyticsAndAdSenseAccountsOverlayNotification />
  <SomeOtherOverlayNotificationWeMakeLater />
</Fragment>

This can be made in assets/js/components/OverlayNotification/OverlayRenderer.js and should be output in DashboardMainApp or similar.

Test Coverage

QA Brief

Changelog entry

mxbclang commented 7 months ago

@tofumatt @marrrmarrr Nitpick here, but should this be "The top earning content metric is now available..." instead of just "Top earning content metric is now available..."? That reads better to me, but LMK what you think!

tofumatt commented 6 months ago

Assigning this a rough estimate of 15 just for planning purposes, but as soon as someone can take on the IB it might need slight adjustment.

tofumatt commented 6 months ago

Increasing this estimate to 19 after adding the Showing only one overlay on-screen at a time section.

eugene-manuilov commented 6 months ago

Showing only one overlay on-screen at a time

Because this pattern will be used for at least two notifications now (and likely will expand in the future), we should implement at least a basic system that prevents multiple overlays from appearing at the same time/on the same page-load.

@tofumatt how about moving this logic into datastores? We have the UI datastore that can control which overlay notification should be shown and control how many of them can be displayed. So, instead of creating a new hook, we can add a new action displayOverlay( overlayID ) to the UI datastore. Then a top level component (OverlayNotificationBase) will be responsible to render overlays that are available in the datastore by calling the getCurrentOverlay (or something like this) selector.

This will also let us remove a need to pack the module related logic in the global component. For example, we can update a resolver in the Analytics 4 module that will "trigger" that overlay when we pull related information. WDYT?

tofumatt commented 6 months ago

@eugene-manuilov Oh, thanks, that's a much better design! 👌🏻

I'll amend the IB here with that approach 👍🏻

tofumatt commented 6 months ago

Having thought about it more, I'm actually pretty sure the existing system would meet those requirements largely. The hook is an abstraction that allows components to easily setup the logic to show/hide themselves and abstracts the calls needed, but ultimately the component that will render is controlled by the datastore… it would still be possible to trigger a particular overlay with:

dispatch( CORE_UI ).setValue( OVERLAY_NOTIFICATION_TO_SHOW, 'someOverlay' );

And I think we'd want to have an OverlayRenderer component that outputs all overlays, which will allow the "active" one (if one exists) to be output, eg:

<Fragment>
  <LinkAnalyticsAndAdSenseAccountsOverlayNotification />
  <SomeOtherOverlayNotificationWeMakeLater />
</Fragment>

I've added that last bit to the IB for clarity, but when I tried making the datastore responsible for selecting the component directly I then needed to create a map of values-to-components, which felt pretty similar to what I've proposed, just in a different way. But the approach I'm proposing means that for simpler notifications like this one, it can run its logic for displaying inside itself, but there's still the ability to dispatch an action elsewhere to trigger a different notification.

eugene-manuilov commented 6 months ago

Ok, let it be it. IB ✔️

zutigrm commented 6 months ago

Note, since this issue will introduce the logic for showing only one overlay, it can be used to adapt #8238 notification that should not be shown if the callout from this issue is shown

zutigrm commented 6 months ago

I created new issue #8335 for updating subtle notification to check if overlay notification is showing, in order not to increase the scope of this issue, as well as QA/testing requirements.

tofumatt commented 6 months ago

I created new issue #8335 for updating subtle notification to check if overlay notification is showing, in order not to increase the scope of this issue, as well as QA/testing requirements.

I've closed that, as the ACs for this issue include that point and it's an important part of this issue, so it will need to be done as part of this 🙂

(Also, the conditions to render each notification should be mutually exclusive so it won't happen, but I may check to see if one is rendered to prevent the other from rendering, just in case 😄)

wpdarren commented 5 months ago

QA Update: ⚠️

@tofumatt I have two observations:

  1. Should the overlay appear on the entity dashboard? We don't have any Monetization widgets on this dashboard, so I wanted to check and get your thoughts.

image

  1. The AC says:

On mobile, it can probably span the full-width of the screen.

I see the overlay on mobile in the bottom right-hand side, but I wanted to check if it should display full-width.

image

tofumatt commented 5 months ago

@wpdarren Oh, it shouldn't appear on the entity page, that's a good point. I thought I checked for that but maybe I forgot to do it as part of this issue 🤔

I think the right-aligned on mobile is fine, that was just a suggestion really but not a requirement. But if it's appearing on the entity dashboard please move this back to Execution and I'll fix that 👍🏻

wpdarren commented 5 months ago

QA Update: ❌

The overlay is appearing on the entity dashboard, so sending it back to @tofumatt to fix.

techanvil commented 5 months ago

Back with you for another pass, @wpdarren.

wpdarren commented 5 months ago

QA Update: ✅

Verified

Screenshots ![image](https://github.com/google/site-kit-wp/assets/73545194/c37f92a7-186c-46f1-9096-27e92a7b6869) ![image](https://github.com/google/site-kit-wp/assets/73545194/c8e59d65-b718-4d26-9b37-e5323e6767c0)