Issue / 8458 Add GA Events for Ads Notification #8474

10upsimon · 2024-04-04T07:59:42Z

Summary

Addresses issue:

Add GA Events for Ads notification #8458

Relevant technical choices

Updated SettingsNotice to accept dismissCallback prop that calls said callback function when the notices is dismissed via said UI element
Updated AdsConversionIDSettingsNotice
- Created callback functions to track GA events for view and dismiss actions
- Passed dismissal callback function as new dismissCallback prop added to SettingsNotice
Created new test suite for SettingsNotice component as SettingsNotice.test.js
- Tested that the dismissCallback function fires as expected during notice dismissal
- Did not test trackEvent functionality as that is isolated from SettingsNotice and handled within a separate component that merely calls SettingsNotice, in this case AdsConversionIDSettingsNotice

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…n click.

…tion and track events for view and dismiss actions.

… dismissal.

github-actions · 2024-04-04T08:05:51Z

Build files for 21ea00b have been deleted.

github-actions · 2024-04-04T08:06:49Z

Size Change: +640 B (0%)

Total Size: 1.42 MB

Filename	Size	Change
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	57.3 kB	+18 B (0%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	34.1 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-api-********************.js`	10.1 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.88 kB	+4 B (0%)
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-data-********************.js`	2.18 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.1 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB	+3 B (0%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	24.3 kB	+1 B (0%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	66.8 kB	+19 B (0%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	91.2 kB	+20 B (0%)
`./dist/assets/js/googlesitekit-modules-ads-********************.js`	18.7 kB	+28 B (0%)
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	111 kB	+38 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	112 kB	+446 B (0%)
`./dist/assets/js/googlesitekit-modules-********************.js`	21.6 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	57.9 kB	+26 B (0%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	30.1 kB	+29 B (0%)
`./dist/assets/js/googlesitekit-polyfills-********************.js`	379 B	+1 B (0%)
`./dist/assets/js/googlesitekit-settings-********************.js`	59.5 kB	+13 B (0%)
`./dist/assets/js/googlesitekit-splash-********************.js`	71.7 kB	-10 B (0%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	46.8 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-vendor-********************.js`	316 kB	-2 B (0%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	33.3 kB	-4 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	60.1 kB	+13 B (0%)
`./dist/assets/js/runtime-********************.js`	1.3 kB	-1 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	53.2 kB
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.2 kB
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.48 kB
`./dist/assets/js/29-********************.js`	2.8 kB
`./dist/assets/js/30-********************.js`	2.28 kB
`./dist/assets/js/31-********************.js`	3.72 kB
`./dist/assets/js/32-********************.js`	930 B
`./dist/assets/js/33-********************.js`	888 B
`./dist/assets/js/34-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	778 B
`./dist/assets/js/googlesitekit-activation-********************.js`	23.8 kB
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	19.1 kB
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	10 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	22.4 kB

_{compressed-size-action}

zutigrm

Thanks @10upsimon LGTM

I updated muteFetch so id doesn't go back and forth for a very small change that I suggested. I also expanded a bit on QAB to include way for setting the Ads Conversion ID field, since there is no more visible field, it will need a small snippet to update the setting.

assets/js/components/SettingsNotice/SettingsNotice.test.js

nfmohit

Excellent work here, thank you @10upsimon. I've left a couple comments, which once addressed, we should be able to get this in.

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js

assets/js/components/SettingsNotice/SettingsNotice.test.js

…b.com:google/site-kit-wp into issue/8458-add-ga-events-for-ads-notification.

…output.

…on notice.

nfmohit

Hi @10upsimon. Thank you for addressing the feedback. I apologise, I have found one additional concern regarding the view_notification trackEvent call, and I have left two other minor comments. Please let me know what you think, thanks!

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js

assets/js/components/SettingsNotice/SettingsNotice.test.js

…sehooks-ts.com/react-hook/use-intersection-observer#api.

…ttps://usehooks-ts.com/react-hook/use-intersection-observer#api." This reverts commit c5e7406.

…b.com:google/site-kit-wp into issue/8458-add-ga-events-for-ads-notification.

nfmohit

Thank you so much for the excellent work, @10upsimon. Thank you for the assist, @techanvil!

I have left what I believe is my last set of very minor feedback, mostly cosmetic and adherence to best practices. Let me know what you think, thanks!

assets/js/components/SettingsNotice/SettingsNoticeWithIntersectionObserver.js

nfmohit · 2024-04-15T21:44:08Z

assets/js/components/SettingsNotice/SettingsNoticeWithIntersectionObserver.js

+// TODO: This could renamed/moved to be more generic e.g. `ComponentWithIntersectionObserver`,
+// or refactored to a HOC.
+function SettingsNoticeWithIntersectionObserver( {


As @techanvil mentioned here, let's actually address this comment now and turn this into a more generic component, as this functionality doesn't need to be specific to the SettingsNotice component, I think. ComponentWithIntersectionObserver sounds great!

@nfmohit @techanvil I have addressed this as a new ComponentWithIntersectionObserver component as per recent commits. I simply merely implemented it as a component with children, and in testing this seemed to work fine. Any concerns with this? See the commit at 32a7df0

LGTM! I'll keep this open for @techanvil.

Hey @10upsimon, thanks for addressing that. However, I notice that the approach you've taken introduces a new div element to wrap the children.

Seeing as the longer term intention here is to address the problem with useIntersection(), at which point we can remove this workaround, I'd rather we don't introduce additional markup via ComponentWithIntersectionObserver which will then be removed when the component is stripped out.

This can be addressed in quite a straightforward manner by rewriting the component as a HOC and passing the ref directly to the wrapped component.

I've had a quick go at this myself, creating withIntersectionObserver() and removing ComponentWithIntersectionObserver.

Although we usually cover our HOC functions with tests, I think we can skip it on this occasion, as long as we're clear about the fact it's just an interim measure.

On that note, it looks like this point in my previous comment has been missed:

Simon, if you haven't already, please can you file a followup issue to provide an improved version of useIntersection() and replace our current use of the version from react-use?

So, please can you do the following?

Take a look at the HOC implementation that I've provided and see what you think, if you are happy with it I'll get this merged.

File that GH issue to make sure we do follow up and address the issue with useIntersection().

Cheers!

Cc @nfmohit

Thanks @techanvil I took a look at your change, and while I need to understand it a little better, I get the gist of it and I tested it and the event still fires correctly, so I'm happy with those changes.

I have indeed added it to my TODO list to create that issue, I did not realise the creation of said issue was a blocker for this PR to be approved, apologies for that, I'll get onto that soon. If we can move this ahead so long, that will help a lot.

Thanks @10upsimon! As discussed on Slack, creating the followup isn't a blocker for this, I just wanted to make sure it's on your radar.

I'll get this one merged, cheers!

assets/js/components/SettingsNotice/SettingsNoticeWithIntersectionObserver.js

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js

…king oriented.

…mponentWithIntersectionObserver component for easier reuse.

…rsion ID settings notice.

…tionObserver component.

…tion.

…ew div wrapper from ComponentWithIntersectionObserver component.

nfmohit

Thank you @10upsimon. This looks excellent. I have pointed out some minor improvements, so minor that I've applied them myself. I hope that is okay with you, thanks!

assets/js/components/SettingsNotice/SettingsNotice.js

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js

nfmohit

LGTM, thank you @10upsimon!

I'll let @techanvil do the honours here and do a final MR and respond to this question. Thanks!

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js

techanvil

Thanks @10upsimon, this is looking good but please can you review my comment here? #8474 (comment)

techanvil

LGTM, nice one @10upsimon!

10upsimon added 3 commits April 4, 2024 09:50

Add dismissCallback prop and propType, call dismiss callback on butto…

bd134de

…n click.

Pass dismissCallback prop to SettingsNotice, define new callback func…

01df320

…tion and track events for view and dismiss actions.

Add test suite for SettingsNotice, track that callback event fires on…

ec93dd8

… dismissal.

Remove unedeed param.

abed954

zutigrm approved these changes Apr 4, 2024

View reviewed changes

assets/js/components/SettingsNotice/SettingsNotice.test.js Outdated Show resolved Hide resolved

nfmohit requested changes Apr 5, 2024

View reviewed changes

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js Outdated Show resolved Hide resolved

assets/js/components/SettingsNotice/SettingsNotice.test.js Outdated Show resolved Hide resolved

10upsimon added 3 commits April 8, 2024 09:38

Merge branch 'issue/8458-add-ga-events-for-ads-notification' of githu…

5227d29

…b.com:google/site-kit-wp into issue/8458-add-ga-events-for-ads-notification.

Remove mock UI wrapper and mock context provider from test component …

ac9ab69

…output.

Add conditional view_notification logic for ads conversion ID migrati…

d006425

…on notice.

10upsimon requested a review from nfmohit April 8, 2024 08:09

10upsimon assigned nfmohit Apr 8, 2024

nfmohit requested changes Apr 8, 2024

View reviewed changes

nfmohit removed their assignment Apr 9, 2024

10upsimon and others added 8 commits April 12, 2024 12:07

Add forwardRef to SettingsNotice component.

836b221

Track view_notification event only if notice should be shown.

94e1c6c

Testing the useIntersectionObserver() implementation from https://u…

c5e7406

…sehooks-ts.com/react-hook/use-intersection-observer#api.

Revert "Testing the useIntersectionObserver() implementation from h…

0c750a6

…ttps://usehooks-ts.com/react-hook/use-intersection-observer#api." This reverts commit c5e7406.

Revert to useIntersection() from react-use, with workaround.

1bcb01d

Merge branch 'issue/8458-add-ga-events-for-ads-notification' of githu…

7828d5c

…b.com:google/site-kit-wp into issue/8458-add-ga-events-for-ads-notification.

Remove console debug lines.

654236a

Swap dimsissed notice key for test key, as opposed to actual store key.

b8d25c1

10upsimon requested a review from nfmohit April 15, 2024 06:30

nfmohit requested changes Apr 15, 2024

View reviewed changes

10upsimon added 5 commits April 16, 2024 21:07

Correct dates, change trackingRef naming to inViewRef to be less trac…

e44df4e

…king oriented.

Add period to end of line comment.

5f45df5

Refactor SettingsNoticeWithIntersectionObserver to be more generic Co…

32a7df0

…mponentWithIntersectionObserver component for easier reuse.

Utilize new ComponentWithIntersectionObserver component for ads conve…

ca729c6

…rsion ID settings notice.

Remove default export of no longer present SettingsNoticeWithIntersec…

b78c6e5

…tionObserver component.

Merge branch 'develop' into issue/8458-add-ga-events-for-ads-notifica…

f6fb450

…tion.

10upsimon requested review from nfmohit and techanvil April 16, 2024 19:39

10upsimon and others added 3 commits April 16, 2024 21:51

Update snapshot for AdsConversionIDSettingsNotice.test.js following n…

878e2c2

…ew div wrapper from ComponentWithIntersectionObserver component.

Remove forwardRef from SettingsNotice.

91fcf2d

Apply minor improvements.

790dd42

nfmohit requested changes Apr 16, 2024

View reviewed changes

assets/js/components/SettingsNotice/SettingsNotice.js Show resolved Hide resolved

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js Outdated Show resolved Hide resolved

nfmohit approved these changes Apr 16, 2024

View reviewed changes

tofumatt changed the base branch from develop to main April 16, 2024 22:56

Replace ComponentWithIntersectionObserver with a HOC.

30c96fa

techanvil reviewed Apr 17, 2024

View reviewed changes

assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js Outdated Show resolved Hide resolved

techanvil added 3 commits April 17, 2024 10:24

Remove unneeded anonymous function.

28ae063

Add an explanatory comment to withIntersectionObserver().

acc288a

Update AdsConversionIDSettingsNotice snapshot.

21ea00b

techanvil requested changes Apr 17, 2024

View reviewed changes

10upsimon requested a review from techanvil April 17, 2024 12:11

techanvil approved these changes Apr 17, 2024

View reviewed changes

techanvil merged commit 2712175 into main Apr 17, 2024
24 checks passed

techanvil deleted the issue/8458-add-ga-events-for-ads-notification branch April 17, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue / 8458 Add GA Events for Ads Notification #8474

Issue / 8458 Add GA Events for Ads Notification #8474

10upsimon commented Apr 4, 2024 •

edited by techanvil

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

zutigrm left a comment

nfmohit left a comment

nfmohit left a comment

nfmohit left a comment

nfmohit Apr 15, 2024

10upsimon Apr 16, 2024

nfmohit Apr 16, 2024

techanvil Apr 17, 2024 •

edited

Loading

10upsimon Apr 17, 2024

techanvil Apr 17, 2024

nfmohit left a comment

nfmohit left a comment

techanvil left a comment

techanvil left a comment

Issue / 8458 Add GA Events for Ads Notification #8474

Issue / 8458 Add GA Events for Ads Notification #8474

Conversation

10upsimon commented Apr 4, 2024 • edited by techanvil Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Apr 4, 2024 • edited Loading

github-actions bot commented Apr 4, 2024 • edited Loading

zutigrm left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit Apr 15, 2024

Choose a reason for hiding this comment

10upsimon Apr 16, 2024

Choose a reason for hiding this comment

nfmohit Apr 16, 2024

Choose a reason for hiding this comment

techanvil Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

10upsimon Apr 17, 2024

Choose a reason for hiding this comment

techanvil Apr 17, 2024

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

techanvil left a comment

Choose a reason for hiding this comment

techanvil left a comment

Choose a reason for hiding this comment

10upsimon commented Apr 4, 2024 •

edited by techanvil

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

techanvil Apr 17, 2024 •

edited

Loading