Add GA events for "Top Earning Pages" widget about site-kit-wp HOT 9 OPEN

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

from site-kit-wp.

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. 👍🏻

from site-kit-wp.

QA Update ⚠️

• Tested on dev environment.
• Verified that View_notification and 'click_learn_more_link' events are getting trigger as per AC.

@benbowler Since #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?

from site-kit-wp.

QA Update ✅

• Tested on dev environment.
• Verified that View_notification and 'click_learn_more_link' and 'view_widget' events are getting trigger as per AC.

from site-kit-wp.

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

from site-kit-wp.

• 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".

from site-kit-wp.

Ok, thanks. IB ✔️

from site-kit-wp.

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?

from site-kit-wp.

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:

Over to CR now.

from site-kit-wp.