Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
15 tasks
tofumatt opened this issue Feb 8, 2024 · 15 comments
Closed
15 tasks
Labels
javascript Pull requests that update Javascript code Module: Analytics Google Analytics module related issues P1 Medium priority Sp Wk 1 Issues to be completed in the first week of the assigned sprint Type: Enhancement Improvement of an existing feature

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Feb 8, 2024

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

        const supportURL = useSelect( ( select ) =>
                select( CORE_SITE ).getGoogleSupportURL( {
                        path: '/adsense/answer/6084409',
                } )
        );
  • The secondary CTA should dismiss the overlay, causing it to no longer appear.
  • Copy:
    • Top heading: See which content earns you the most
    • Content: Link your Analytics and AdSense accounts to find out which content brings you the most revenue.
    • Primary CTA: Learn how
    • Secondary CTA: Maybe later

Implementation Brief

  • Create a new folder to store this type of notification, eg. assets/js/components/OverlayNotification
  • Create a new component (OverlayNotificationBase) in a file (assets/js/components/OverlayNotification/base.js) for the structure of this notification, it can borrow a fair bit of style from the surveys components eg.
    <Slide
    direction="up"
    in={ animateSurvey }
    onExited={ handleAnimationOnExited }
    >
    <div className="googlesitekit-survey">
    / to start with, but created in its own file assets/sass/components/overlay-notification.scss
    • The component should support rendering content inside it, for now it can just be children without any enforced structure; we can change this in the future after we have a few of these kinds of notifications and notice a pattern 😄
  • Create an overlay notification (assets/js/components/OverlayNotification/LinkAnalyticsAndAdSenseAccountsOverlayNotification.js) using the OverlayNotificationBase component with this overlay's content, roughly:
<OverlayNotificationBase>
  <svgHeader />
  <Heading>See which content earns you the most</Heading>
  <Text>Link your Analytics and AdSense accounts to find out which content brings you the most revenue.</Text>
  <Button>Learn how</Button>
  <Button>Maybe later</Button>
</OverlayNotificationBase>
  • Each component should reflect the style in the Figma design, using existing classNames/styles where appropriate/possible. New ones can be created in assets/sass/components/overlay-notification.scss.
  • A new constant can be made in this file for dismissing this notification (eg. LINK_ANALYTICS_ADSENSE_OVERLAY_DISMISSED = 'link-analytics-and-adsense-overlay';)
  • Both CTA buttons should use const { dismissItem } = useDispatch( CORE_USER ); and dismissItem( LINK_ANALYTICS_ADSENSE_OVERLAY_DISMISSED )
  • The primary CTA should also open a link to the supportURL value mentioned in the ACs, in a new tab/window. (The aria-label for this button should mention to the user it will open in a new window).
  • This component should render null when:
    • The ga4AdSenseIntegration feature flag is disabled
    • The user is viewing a view-only dashboard
    • Analytics and/or AdSense are not connected
    • select( MODULES_ANALYTICS_4 ).getAdSenseLinked() is true
    • select( CORE_USER ).isItemDismissed( LINK_ANALYTICS_ADSENSE_OVERLAY_DISMISSED ) is true

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 };
}
  • Any overlay notification (like the above assets/js/components/OverlayNotification/LinkAnalyticsAndAdSenseAccountsOverlayNotification.js) could use it as such:
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

  • Add test coverage to the useOverlayNotification hook ensuring it does not change the "active" component after it is set, does not set any components to output by default, etc.
  • Add VRTs for the LinkAnalyticsAndAdSenseAccountsOverlayNotification component.

QA Brief

  • Setup Site Kit with Analytics and AdSense
  • Preferably the ones that are not linked, but since adSenseLinked is not receiving real data, it will already be false by default
  • Navigate to the dashboard, notification should not be showing without feature flag
  • Enable ga4AdSenseIntegration feature flag
  • Navigate to the dashboard, and verify that overlay notification is showing properly
  • Clicking Learn how button should open a help article in new tab and dismiss notification
  • Clicking on Maybe later should just dismiss the notification
  • Once dismissed, it should not be showing on next page reload

Changelog entry

  • Display overlay notification to prompt AdSense + Analytics 4 users to link their accounts.
@tofumatt tofumatt added P1 Medium priority javascript Pull requests that update Javascript code labels Feb 8, 2024
@tofumatt tofumatt self-assigned this Feb 8, 2024
@tofumatt tofumatt added the Module: Analytics Google Analytics module related issues label Feb 8, 2024
@tofumatt tofumatt added the Next Up Issues to prioritize for definition label Feb 8, 2024
@bethanylang
Copy link
Collaborator

@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
Copy link
Collaborator Author

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 tofumatt assigned tofumatt and unassigned tofumatt Feb 22, 2024
@tofumatt
Copy link
Collaborator Author

tofumatt commented Feb 23, 2024

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

@bethanylang bethanylang added the Sp Wk 1 Issues to be completed in the first week of the assigned sprint label Feb 23, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 26, 2024
@eugene-manuilov
Copy link
Collaborator

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
Copy link
Collaborator Author

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

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

@tofumatt
Copy link
Collaborator Author

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.

@tofumatt tofumatt assigned eugene-manuilov and unassigned tofumatt Feb 27, 2024
@eugene-manuilov
Copy link
Collaborator

Ok, let it be it. IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 27, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Feb 27, 2024

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 zutigrm self-assigned this Feb 28, 2024
@zutigrm zutigrm added the Type: Enhancement Improvement of an existing feature label Feb 28, 2024
@bethanylang bethanylang removed the Next Up Issues to prioritize for definition label Feb 28, 2024
@eugene-manuilov eugene-manuilov removed their assignment Mar 5, 2024
@tofumatt tofumatt self-assigned this Mar 6, 2024
@tofumatt
Copy link
Collaborator Author

tofumatt commented Mar 6, 2024

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 😄)

@tofumatt tofumatt removed their assignment Mar 7, 2024
@kuasha420 kuasha420 assigned kuasha420 and tofumatt and unassigned kuasha420 Mar 13, 2024
@tofumatt tofumatt assigned kuasha420 and unassigned tofumatt Mar 13, 2024
@kuasha420 kuasha420 removed their assignment Mar 14, 2024
@wpdarren wpdarren self-assigned this Mar 14, 2024
@wpdarren
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

QA Update: ❌

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

@techanvil
Copy link
Collaborator

Back with you for another pass, @wpdarren.

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified

  • With ga4AdSenseIntegration feature flag disabled, the notification does not be show.
  • With ga4AdSenseIntegration feature flag enabled, the overlay notification is showing properly.
  • Clicking Learn how button opens a help article in new tab and dismisses the notification.
  • Clicking on Maybe later dismisses the notification and is not showing on next page reload.
  • The overlay notification only appears on the main dashboard - not on view-only and entity dashboard.
  • On smaller viewports, the overlay notification appears on the right-side.
Screenshots

image
image

@wpdarren wpdarren removed their assignment Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code Module: Analytics Google Analytics module related issues P1 Medium priority Sp Wk 1 Issues to be completed in the first week of the assigned sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants