-
Notifications
You must be signed in to change notification settings - Fork 561
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
CommercialMetrics includes ad blocker use #24191
Conversation
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’d like this to be a little more robust. Relying on the shape of the window object, which we declare in a gobal.d.ts
file, feels a little brittle.
static/src/javascripts/projects/commercial/commercial-metrics.ts
Outdated
Show resolved
Hide resolved
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.
Annoying that the TS check doesn’t pass.
}; | ||
|
||
confiant?: Confiant; | ||
|
||
googletag?: googletag.Googletag; |
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 fixes the typescript errors in static/src/javascripts/projects/commercial/modules/dfp/prepare-googletag.ts
Seen on PROD (merged by @zekehuntergreen 11 hours, 49 minutes and 46 seconds ago)
|
What does this change?
sendCommercialMetrics
so that it accepts an optional parameteradBlockerInUse
detectAdblock
so thatwindow.guadian.adBlockers.active
is always a boolean or undefined. Before the change, it's value could be undefined, true, false, or empty string.Does this change need to be reproduced in dotcom-rendering ?
Screenshots
N/A
What is the value of this and can you measure success?
We'd like to collect data on our readers' use of ad blockers
Checklist
Does this affect other platforms?
Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?
Does this change break ad-free?
Does this change update the version of CAPI we're using?
Accessibility test checklist
Tested