-
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
Add Outstream Renderer for Yieldlab Adapter #3910
Conversation
if (playersize) { | ||
bidResponse.width = playersize[0] | ||
bidResponse.height = playersize[1] | ||
} |
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.
Hello @mirkorean
Looks good, only a small issue.
playersize
will come as an array of size arrays here if it comes. Prebid translates it to this. Here are some possible settings an Ad Unit can have and how your getPlayerSize function will return:
No playerSize
defined by user:
sizes: [[640, 480]],
mediaTypes: {
video: {
context: 'outstream'
}
}
getPlayerSize(bidRequest)
will return undefined
here.
Which you check and would not set the width and height which would default to you customsize
thing above
User sets playerSize to a single size array
mediaTypes: {
video: {
playerSize: [640, 480],
context: 'outstream'
}
},
Prebid takes this input size and translates it to an array of size arrays before sending to the adapters as seen here
getPlayerSize(bidRequest)
will return [ [ 640, 480 ] ]
here.
Which then causes your code to set:
bidResponse.width
= [ 640, 480 ]
bidResponse. height
= undefined
This will also happen if user sets
mediaTypes: {
video: {
playerSize: [[640, 480]],
context: 'outstream'
}
},
So the recommendation here is to add some logic to grab the sizes width and height correctly.
function getPlayerSize (format) {
return utils.deepAccess(format, 'mediaTypes.video.playerSize') ? mediaTypes.video.playerSize[0] : undefined;
}
Maybe?
Up to you exactly how to do this (I did not even really test or look at the example I gave.)
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.
Thanks for your review, much appreciated. I commited a fix for this.
Prebid is translating the playerSize to an array of arrays, so we have to return accordingly
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.
- changed the getPlayerSize function like you proposed
- added a test for it
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.
Awesome!~!
Thanks
* Add Outstream Renderer * Fix playerSize overwrite Prebid is translating the playerSize to an array of arrays, so we have to return accordingly
* Add Outstream Renderer * Fix playerSize overwrite Prebid is translating the playerSize to an array of arrays, so we have to return accordingly
Type of change
Description of change