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

Add GA events for "Top Earning Pages" widget #8212

Closed tofumatt closed 6 months ago

tofumatt commented 7 months ago

Feature Description

GA events (eg. trackEvent calls) should be added to the new GA4-powered "Top Earning Pages" widget in its "setup"/CTA version. There should be one when a user views the widget, and one when they click the "Learn more" link in the widget, which serves as the CTA button.


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

Acceptance criteria

CTA widget screenshot:

image

Implementation Brief

Test Coverage

QA Brief

To QA the linked state before #8049 is merged you can run googlesitekit.data.dispatch('modules/analytics-4').setAdSenseLinked(true) in the developer console and you will see the widgets load the report data if there are earnings for your test site and you can test the tracking events in this state.

Changelog entry

ivonac4 commented 7 months ago

@tofumatt do you mind adding an estimate to this ticket if possible?

eugene-manuilov commented 7 months ago
  • [ ] Using the useRef hook, add a ref to the rendered Widget components (for both Widget components that renders AdSenseLinkCTA and the unconditionally rendered widget content).

@nfmohit, why not to implement event tracking in the CTA itself? The same way how we do it in the KeyMetricsCTAContent component. In this case, we won't need to change the Widget component.

In assets/js/googlesitekit/widgets/components/Widget.js:

  • [ ] Accept additional (spread) props in the Widget component and pass any additional props passed into Widget to the div.googlesitekit-widget element.

If we really need this, then the component should be updated using the forwardRef approach. https://react.dev/reference/react/forwardRef

  • In the rendered AdSenseLinkCTA component, add an onClick prop and track the click_learn_more_link event according to the ACs.

Let's expand this a little bit more and add more details to make sure that we have an agreement as to what we mean by "according to the ACs".

nfmohit commented 7 months ago

Hi @eugene-manuilov. As discussed, I have mentioned the usage of forwardRef, and addressed other feedback. IB updated, thank you!

eugene-manuilov commented 7 months ago

Ok, thanks. IB ✔️

benbowler commented 7 months ago

I have been working on this today and have implemented it with one small addition to the IB; updating assets/js/googlesitekit/widgets/util/get-widget-component-props.js to forward the ref through to the underlaying Widget component.

However while testing this I found that if I scrolled down on the dashboard slowly the event would not trigger. It would only trigger if I scrolled to this section quickly, or clicked on "Monetization" then scrolled down from there. (I have a video I can share of this occurring I will share directly).

Debugging this a little, it seems the DashboardTopEarningPagesWidgetGA4 component does not get the updated node reference to the underlaying Widget in this case as it's not rendered immediately and useRef does not trigger re-rendering when the underlaying node is created.

I could not repeat this bug with the /DashboardPageSpeed widget.


I began looking at using useCallback instead of useRef, as in this React doc, to get the updated node, however this would not work with the useIntersection hook so would required much additional work.

The simplest solution would instead to wrap the components with <div ref={ trackingRef }></div>, within the DashboardTopEarningPagesWidgetGA4, avoiding the use of forwardRef entirely.


@tofumatt @eugene-manuilov, what is the best approach in your view?

tofumatt commented 7 months ago

Huh, good catch!

The simplest solution would instead to wrap the components with <div ref={ trackingRef }></div>, within the DashboardTopEarningPagesWidgetGA4, avoiding the use of forwardRef entirely.

Which components would need wrapping exactly? We need Widget/WidgetNull to be the top-level components that are returned for the widget layout code to work, but if you need to wrap anything insight I don't have a problem with that. 👍🏻

benbowler commented 7 months ago

I've tried wrapping the internals of both of the Widgets with <div ref={ trackingRef }></div>, however the same bug occurs. trackingRef does not pass the correct dom element to IntersectionObserver.

The way I could resolve this was to intercept the ref function and force the DashboardTopEarningPagesWidgetGA4 component to re-render when it received the DOM element:

https://github.com/google/site-kit-wp/blob/034818eb3c27089d23bcf6a17eaa1b0568de8765/assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidgetGA4.js#L125-L135

Over to CR now.

mohitwp commented 6 months ago

QA Update ⚠️

@benbowler Since https://github.com/google/site-kit-wp/issues/8049 has not been merged yet we are using the code snippet provided under QAB to load the widget. However, I have noticed that when users view the 'Top earning pages' widget then event 'view_widget' is not being triggered. . Could you please investigate whether the event is not being triggered because we are forcibly displaying it through the code snippet, or if it is actually an issue with the event not being triggered under any circumstances?

image

`View_notification` ![image](https://github.com/google/site-kit-wp/assets/94359491/ef6018c5-1865-4fea-a4f8-bc341a37f776) ![image](https://github.com/google/site-kit-wp/assets/94359491/8a061c21-31da-4a7a-a27a-032964c9ac8b) `'click_learn_more_link'` ![image](https://github.com/google/site-kit-wp/assets/94359491/4dced3f5-0098-4ccd-99e6-91325e39ee79) ![image](https://github.com/google/site-kit-wp/assets/94359491/fdbac4bd-a6e9-473b-b9cb-5c7255b9908e)
mohitwp commented 6 months ago

QA Update ✅

`View_notification` ![image](https://github.com/google/site-kit-wp/assets/94359491/ef6018c5-1865-4fea-a4f8-bc341a37f776) ![image](https://github.com/google/site-kit-wp/assets/94359491/8a061c21-31da-4a7a-a27a-032964c9ac8b) `'click_learn_more_link'` ![image](https://github.com/google/site-kit-wp/assets/94359491/4dced3f5-0098-4ccd-99e6-91325e39ee79) ![image](https://github.com/google/site-kit-wp/assets/94359491/fdbac4bd-a6e9-473b-b9cb-5c7255b9908e) `'view_widget'` ![image](https://github.com/google/site-kit-wp/assets/94359491/6319ded1-09f3-40cd-b5fd-7c2f67ae5163) ![image](https://github.com/google/site-kit-wp/assets/94359491/0ec05ba1-427b-4b28-bf57-247bbe214ecc) ![image](https://github.com/google/site-kit-wp/assets/94359491/4f19ab44-2116-4656-979c-7866200cd0a8) ![image](https://github.com/google/site-kit-wp/assets/94359491/7def52f8-ef10-432a-bcce-e49179a194ef)