-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Allow user to override which gpt slots should be targeted at invocation #2562
Allow user to override which gpt slots should be targeted at invocation #2562
Conversation
Wopps. Not ment to close this as its very valid IMHO and backwards compatible |
src/prebid.js
Outdated
@@ -156,9 +156,10 @@ $$PREBID_GLOBAL$$.getBidResponsesForAdUnitCode = function (adUnitCode) { | |||
/** | |||
* Set query string targeting on one or more GPT ad units. | |||
* @param {(string|string[])} adUnit a single `adUnit.code` or multiple. | |||
* @param {function(object)} customSlotMatching gets a GoogleTag slot and returns a filter 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.
Can you update the description and add a usage example here?
ie, this is a a filter function that if passed will override the default matching for a slot.
customSlotMatching(slot) => slot.getAdUnitPath() === 'someIdPassed'
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.
Overall I like this approach much better, nice work! small change requested for updating the JSDoc.
Thanks for the feedback, will fix! |
Also needs a PR opened here for the new function signature: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/publisher-api-reference-old.md#module_pbjs.setTargetingForGPTAsync Once that's open and linked the |
Thanks for the pointer! |
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.
LGTM
@mkendall07 is this ready for merging or did I miss something? |
…on (prebid#2562) * Allow user to override which gpt slots should be targeted at invocation time * Added filter example
…on (prebid#2562) * Allow user to override which gpt slots should be targeted at invocation time * Added filter example
…on (prebid#2562) * Allow user to override which gpt slots should be targeted at invocation time * Added filter example
…on (prebid#2562) * Allow user to override which gpt slots should be targeted at invocation time * Added filter example
…on (prebid#2562) * Allow user to override which gpt slots should be targeted at invocation time * Added filter example
…on (prebid#2562) * Allow user to override which gpt slots should be targeted at invocation time * Added filter example
Type of change
Description of change
We use the following flow:
We start a auction as early as possible. Without knowing where in the feed we're gonna place it. So depending on the outcome of the auction and how long it took we might inject it into a placement defined further down the page in order to now burn views above the fold.
The current semantics of setTargeting requires that we shall know a head of time what gptSlot the targeting should apply for because its looking into the auction object.
But allowing us to pass in an custom slot matching filter we can allow the targeting to explicitly use this slot when setting targeting
We use this in order not to place ads above fold and insert them to a better position instead of everything just being very static.
Other information
Ref #1876 the old attempt, this is way cleaner and just follows along the new 1.x branch
cc: @mkendall07