-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add slot targeting to indicate teads eligibility #1603
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d0c49a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Ad load time test resultsFor Test conditions:
|
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.
That looks great Emma ✨ I really enjoyed reading your clean code! It's great to avoid GAM configuration and setup as much as we could 👏🏼
]; | ||
|
||
const isEligibleForTeads = (slotId: string) => { | ||
const { page } = window.guardian.config; |
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.
Minor:
const { page } = window.guardian.config; | |
const { contentType, pageId, isSensitive } = window.guardian.config.page; |
const instancesOfBannedKeywords = urlKeywords.filter((keyword) => | ||
bannedUrlKeywords.includes(keyword), | ||
); |
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.
Minor:
const instancesOfBannedKeywords = urlKeywords.filter((keyword) => | |
bannedUrlKeywords.includes(keyword), | |
); | |
const hasBannedKeywords: boolean = urlKeywords.some((keyword) => | |
bannedUrlKeywords.includes(keyword), | |
); |
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.
Hadn't seen this before - definitely neater, thanks!
src/define/Advert.ts
Outdated
if (isEligibleForTeads(adSlotNode.id)) { | ||
slotTargeting['teadsEligible'] = 'true'; | ||
} else { | ||
slotTargeting['teadsEligible'] = 'false'; | ||
} |
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.
Most other slot targeting is set in defineSlot
might that be a better place to set this also?
@@ -0,0 +1,83 @@ | |||
import { isEligibleForTeads } from './teads-eligibility'; | |||
|
|||
describe('Teads Eligibility', () => { |
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.
Great to see some unit tests 🥳
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.
Nice work ✨
Just some minor suggestions
What does this change?
Adds slot level targeting to add a key word to indicate if a slot is eligible for teads or not. The checks in this diagram have been implemented as code checks to reduce the logic we need to add in GAM.
Why?
At the moment a lot of targeting needs to be added in GAM to determine slots which are eligible for teads. It's complex to set up ads in GAM so that they don't target slots which are eligible for teads. By adding a specific key word we can simplify the targeting set up for teads in GAM.