Conversation
snapwich
left a comment
snapwich
left a comment
There was a problem hiding this comment.
function getWinningBidTargeting also needs to be updated to accept param.
src/targeting.js
Outdated
| pbTargetingKeys.forEach(function(key) { | ||
| // reset only registered adunits | ||
| adUnitCodes.find(function(unit) { | ||
| adUnits.find(function(unit) { |
There was a problem hiding this comment.
This should probably be .forEach as find is just being used here to loop through the adUnits
src/targeting.js
Outdated
| const adUnitCodes = adUnitCode && adUnitCode.length ? [adUnitCode] : $$PREBID_GLOBAL$$._adUnitCodes; | ||
| let adUnitCodes = $$PREBID_GLOBAL$$._adUnitCodes; | ||
| if (typeof adUnitCodes === 'string') { | ||
| adUnitCode = [adUnitCode]; |
There was a problem hiding this comment.
should this be adUnitCodes = [adUnitCode]?
|
Thanks for the quick turnaround looking into this |
src/prebid.js
Outdated
| * @alias module:$$PREBID_GLOBAL$$.setTargetingForGPTAsync | ||
| */ | ||
| $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function (adUnits) { | ||
| $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function (adUnitsCodeArr) { |
There was a problem hiding this comment.
Do you think we should start JSDoc-ing params whenever the types cause confusion?
Renaming it here does help... but in other cases (like getAllTargeting, where the arg might be a string or array) that's not always possible.
There was a problem hiding this comment.
yeah we should. I'll update that
protonate
left a comment
protonate
left a comment
There was a problem hiding this comment.
Currently we have adUnits and _adUnitCodes with the latter being a "filter" of ad units to use in current operation. A subset of ad units gets persisted to _adUnitCodes. This change adds a second "filter" of sorts just when setting targeting, so if the intent is to set targeting on the same subset of ad units for which bids were requested we should use the existing filter _adUnitCodes. If a subset of the subset, as we have here, then the filter will not be persisted such that a subsequent call to setTargeting... will operate on the "full subset" of _adUnitCodes, not the last used subset of codes passed to setTargeting... Is this the intention?
src/prebid.js
Outdated
| * @alias module:$$PREBID_GLOBAL$$.setTargetingForGPTAsync | ||
| */ | ||
| $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function (adUnits) { | ||
| $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function (adUnitsCodeArr) { |
There was a problem hiding this comment.
can adUnitCodes be standard param name, here in place of adUnitsCodeArr?
There was a problem hiding this comment.
Yeah I thought about it. It does make sense
src/targeting.js
Outdated
| else if (utils.isArray(adUnitCode)) { | ||
| return adUnitCode; | ||
| } | ||
| return $$PREBID_GLOBAL$$._adUnitCodes; |
There was a problem hiding this comment.
It should not be possible to get to this point and still have pbjs._adUnitCodes filter undefined but might want to test for that or otherwise assure the return value here, if undefined, is handled.
There was a problem hiding this comment.
good call. I'll address.
snapwich
left a comment
snapwich
left a comment
There was a problem hiding this comment.
LGTM. I think some of the variables names are still kind of wonky, but I think most of this will be resolved when we adopt types, so it can wait for later :)
….23.0 to aolgithub-master * commit '136fc37637749a764070c35c03e7e87a5c157947': (33 commits) Added changelog entry. Implemented passing key values feature. Update code to ESlint rules. Prebid 0.24.1 Release tests: drop ie9 browserstack test Audience Network: separate size from format (prebid#1218) Bugfix/target filtering api fix (prebid#1220) Map sponsor request param to endpoint param (prebid#1219) Increment pre version Probed 0.24.0 Release Beachfront adapter - add ad unit size (prebid#1183) Thoughtleadr adapter - fix postMessage (prebid#1207) When prebid server issues a no-bid response, call addBidResponse for every adUnit requested (prebid#1204) Improvement/timeout xhr (prebid#1172) Add native support (prebid#1072) Improvement/alias queue (prebid#1156) Updated documentaion (prebid#1160) Improvement/prebid iframes amp pages (prebid#1119) Fixes prebid#1114 possible xss issue (prebid#1186) Allowed setTargetingForGPTAsync() to target specific ad unit codes. (prebid#1158) ...
4 participants
Type of change
Description of change
Resolves #1217
Other information
@prebid/core for review