-
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
RockYou Adapter: Added RockYou Adapter supporting Prebid 1.0 #1977
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.
Thanks for the PR, a few changes requested below
@@ -0,0 +1,403 @@ | |||
import { expect } from 'chai'; | |||
import chai from 'chai'; |
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 import isn't needed, line can be dropped
import { expect } from 'chai'; | ||
import chai from 'chai'; | ||
import { spec, internals } from 'modules/rockyouBidAdapter'; | ||
import { ROTATION_ZONE } from 'modules/rockyouBidAdapter'; |
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 isn't used in the test, line can be dropped
|
||
let validCallbacks = ['LOAD', 'IMPRESSION', 'COMPLETE', 'ERROR']; | ||
|
||
for (event in validCallbacks) { |
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.
event
should probably be preceded with const
or let
…orEach for variable scoping.
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. Build failures were due to issue on Travis side and tests pass locally.
@cameronhotchkies Please also submit a PR to the docs repo to add a file for your adapter to the bidders directory so your adapter's params will appear on the bidders page
Thank you. Docs updated at https://github.com/prebid/prebid.github.io/pull/551/files |
modules/rockyouBidAdapter.js
Outdated
if (!utils.isEmpty(bidRequest.sizes)) { | ||
// Currently only the first size is utilized | ||
let firstSize = bidRequest.sizes[0]; | ||
width = firstSize[0]; |
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 won't work if sizes is defined like [300,250]
. You'll need to check for that condition.
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.
just to clarify, sizes can be List[List[X,Y]] or List[X,Y] ?
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's right. There's a helper function you can use: https://github.com/prebid/Prebid.js/blob/master/src/utils.js#L113
Sorry this is a pain. The core should handle this but it's hard to update because every adapter might be doing it differently.
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.
Not a problem. Is the "HxW" vs [H,W] format (as returned by the helper function) more universal?
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 think so. But the [H,W]
| [[H,W]]
format is what DFP uses so that's what our external API looks like. The core should normalize this so each adapter doesn't have to.
@cameronhotchkies |
… definitions to be handled successfully
* 'master' of https://github.com/prebid/Prebid.js: Prebid 1.2.0 Release Use polyfilled includes method (prebid#2061) RockYou Adapter: Added RockYou Adapter supporting Prebid 1.0 (prebid#1977) Optimera Adapter for 1.0. (prebid#1961) Use cross-browser integer check (prebid#2058) Fix skipped test (prebid#2059) Support multiple media formats within a single ad unit (prebid#1991) pre1api module that allows use of deprecated pre1.0 API in Prebid 1.0 (prebid#1976) Colossus SSP header bidding adapter 1.0.0 (prebid#2029) InSkin Bidder Adapter (prebid#2016) Update adapter to prebid v1.0 (prebid#1908) PubMatic 1.0 adapter (prebid#2011)
…1977) * RockYou Adapter: Added RockYou Adapter supporting Prebid 1.0 * RockYou Adapter: Removed extraneous imports, replaced a for loop w/ forEach for variable scoping. * RockYou Adapter: Updated the size handling to allow for multiple size definitions to be handled successfully
Type of change
Description of change
Created a new bidder adapter for RockYou.
mediaTypes: { video: {}}
is set in the adUnit):