-
Notifications
You must be signed in to change notification settings - Fork 916
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
Send scroll event to data layer #15098 #15179
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15179 +/- ##
==========================================
+ Coverage 77.85% 78.35% +0.49%
==========================================
Files 162 162
Lines 8458 8316 -142
==========================================
- Hits 6585 6516 -69
+ Misses 1873 1800 -73 ☔ View full report in Codecov by Sentry. |
815a87b
to
d424a70
Compare
- Adds scroll listener - Adds event to the dataLayer when thresholds are passed - Removes listener after all thresholds are passed
d424a70
to
32b5618
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working well and as expected. Good work! r+
I think it'd also be beneficial to have @alexgibson take one last look before we merge just in case
delete window.dataLayer; | ||
}); | ||
|
||
it('will append the scroll event to the dataLayer', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (non-blocking): it would be nice to cover TrackScroll.scrollListener
in a test, which is where most of the logic for this PR lives? Right now, sendEvent
would not fire if there was an error in the function that calls it?
Co-authored-by: Alex Gibson <[email protected]>
Co-authored-by: Alex Gibson <[email protected]>
387d819
to
9271c09
Compare
Updated except for tests. |
const now = new Date(); | ||
|
||
// if it's been a while since the last one, log it | ||
if (now - lastPush >= pushDelay) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I'm a bit curious why we need to do this frequent date checking / parsing here? This kind of negates the benefit of trying to de-bounce events a bit since it still fires continuously.
Could we try something like this perhaps?
TrackScroll.scrollListener = () => {
clearTimeout(pushTimer);
pushTimer = setTimeout(function () {
TrackScroll.scrollHandler();
}, pushDelay);
};
This seems to work OK when I test locally (GA events are still sent OK after the scroll).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important to log the events intermittently in case someone slowly scrolls down the page and then exits before the timeout finishes. The date check is a throttle instead of a debounce (though, there's also a debounce to catch the final state after scrolling stops). In theory the date check is less resource intensive than executing the full function continuously.
If you're concerned could we try the throttled function for a week and then move to one that is only debounced and compare the data. It's possible the throttle is unnecessary but I worry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important to log the events intermittently in case someone slowly scrolls down the page and then exits before the timeout finishes.
I tried to to this using a pretty standard de-bounce with a 100ms delay, and i couldn't really get it not to send an event before i could close anything. Maybe if I used a keyboard shortcut? It does seem a little unlikely when trying it out practically.
In theory the date check is less resource intensive than executing the full function continuously.
I'm not 100% sure if we need this more complicated mix of throttling and de-bouncing though, when we're only sending a maximum of 4 events?
It's been a while since I've had to think about this stuff, but the way de-bouncing usually works (iiuc) is by returning a closure, and then you can do either a setTimeout
or a date comparison check (similar to what you have) - and all this should happen async, outside of the actual scroll event handler. That way there's no need to execute the full function continuously.
If you're concerned about losing data, then lowering the delay to 100ms would still be a pretty big improvement imho (although 200ms seems to work ok for me still as well). Here's what I mean by returning a closure:
function debounce(func, delay) {
let timer;
return function () {
clearTimeout(timer);
timer = setTimeout(() => {
func.apply(this, arguments);
}, delay);
};
}
const onScroll = debounce(TrackScroll.scrollHandler, 100);
window.addEventListener('scroll', onScroll, false);
One-line summary
Send scroll event to data layer at 25%, 50%, 75% and 90% of the page.
Significant changes and points to review
This alone is not enough to get the events into GA4 - GTM changes must be made too, but publishing this first makes debugging that easier.
Issue / Bugzilla link
Requested in #15098 but people keep asking me for these numbers on campaigns that have finished running and it's too late to get them. So this will add the event to all pages.
Testing