Skip to content
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

Sonobi Adapter - Enable size overrides #1141

Merged
merged 1 commit into from
May 2, 2017

Conversation

Studnicky
Copy link
Member

@Studnicky Studnicky commented Apr 19, 2017

Type of change

  • Feature

Description of change

Enable optional size overrides on a per-slot basis

@dbemiller
Copy link
Contributor

Hey, the code in general looks good to me. I'd recommend a more thorough unit test, though... the ones you have don't really test the content of the changed code at all.

Can you give me some context into why you're adding this, though? An ad's size is it's size... so I'm not sure why sonobi would want to see a different size than the actual one. I'm wondering if there's some use-case not considered by the prebid core code which you're hacking your way around.

Copy link
Collaborator

@protonate protonate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@protonate protonate added this to the Prebid 0.23.0 milestone May 2, 2017
@protonate protonate added the LGTM label May 2, 2017
@Studnicky
Copy link
Member Author

Studnicky commented May 2, 2017

I agree, however, the use case goes as follows:

  • Publisher implements our adapter on a page with a slot defined as [[970, 250], [970, 90], [728, 90]]
  • Publisher calls us and asks why they are getting 970x250 when they only want 728x90
  • We explain to publisher that they can just change the slot definition, because sizes are sizes
  • Publisher refuses to do so but insists we fix it without them changing their slot definitions
  • Well, ok dude, now you can limit our bids to only return 728x90 without changing slot definitions

@dbemiller
Copy link
Contributor

ok.. thanks for the background!

I do suspect there's a more general feature request buried in here... but this'll work for now. We might be able to incorporate something into the prebid 1.0 API, when we're ready to make breaking changes, so that all the adapters won't have to implement it individually.

@dbemiller dbemiller merged commit 441455d into prebid:master May 2, 2017
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request May 4, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (21 commits)
  add lodash as dependency (prebid#1174)
  fix size mapping for s2s (prebid#1175)
  Improve footer styling (prebid#1171)
  Bugfix: internal bids requested overwritten (prebid#1173)
  pre-release version bump
  Prebid 0.23.0 Release
  Yieldbot adapter - multiple requestBids per pageview (prebid#1146)
  Widespace adapter validate size fix (prebid#1140)
  Audience Network: bid when at least one valid slot size (prebid#1148)
  Quantcast adaptor (prebid#1063)
  AOL Adapter - ONE Mobile endpoint implemented. (prebid#1115)
  Prebid Server to Server (prebid#1165)
  Pubgears Header Bidding Adapter (prebid#953)
  remove old adloader#trackPixel (prebid#1159)
  added audit beacon to detect misuse of this bidder.  Detects auctions… (prebid#1134)
  Bidfluence CDN endpoint URL update (prebid#1163)
  AdSupply adapter (prebid#1162)
  Sonobi Adapter - Enable size overrides (prebid#1141)
  Added an editorconfig file to match jshint and jssrc files. (prebid#1147)
  force cpm to be a number (prebid#1161)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants