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

Audience Network: separate size from format #1218

Merged
merged 1 commit into from
May 25, 2017

Conversation

lovell
Copy link
Contributor

@lovell lovell commented May 22, 2017

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

Hello, this change is based on feedback from Prebid users and was commissioned and paid for by Facebook.

Some confusion has arisen due a mixture of size and format values in the Audience Network adaptor. This PR seeks to separate the two concerns by moving format to a parameter of its own instead of overloading the existing concept of size. This prevents formats specific to Audience Network being sent to other bidders as sizes.

There will be a separate PR for the associated documentation.

/cc @wheresbarney

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

[bid.width, bid.height] = size.split('x').map(Number);
}
bid.ad = createAdHtml(placementId, format, bidId);
[bid.width, bid.height] = size.split('x').map(Number);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice bit of code there.

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
Copy link
Collaborator

protonate commented May 25, 2017

When debugging FB error response I noticed that here: https://github.com/lovell/Prebid.js/blob/aba63d940ae7600382139d4f9f9f3f08c5f39709/src/adapters/audienceNetwork.js#L169

any errors are logged using utils.logError. If these error states are expected (such as a "no bid" response) consider using utils.logWarning instead.

@protonate protonate merged commit ea7b5fa into prebid:master May 25, 2017
@lovell lovell deleted the audience-network-size-vs-format branch May 25, 2017 19:29
vzhukovsky added a commit to aol/Prebid.js that referenced this pull request Jul 17, 2017
….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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants