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.

Sign up for GitHub

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

Jump to bottom

Add Yieldlab adapter #1967

Merged
merged 3 commits into from
Jan 25, 2018
Merged

Add Yieldlab adapter #1967

merged 3 commits into from
Jan 25, 2018

Conversation

GEMI
Copy link
Contributor

@GEMI GEMI commented Dec 15, 2017 •
edited
Loading

Type of change

  • New bidder adapter

Description of change

This is an adapter for Yieldlab.

Regular ad:

{
  bidder: 'yieldlab',
  params: {
      placementId: '4206978',
      accountId: '2358365',
      adSize: '800x250'
  }
}

Video ad:

{
code: 'test',
sizes: [[]],
mediaTypes: {
  video: {
    context: 'instream'
  }
},
bids: [{
    bidder: 'yieldlab',
    params: {
      placementId: '4207034',
      accountId: '2358365',
      adSize: '1x1'
    }
  }]
}

@GEMI
Copy link
Contributor Author

GEMI commented Jan 18, 2018

Ping!

jaiminpanchal27
jaiminpanchal27 requested changes Jan 22, 2018
View reviewed changes
Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 left a comment

Choose a reason for hiding this comment

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

@GEMI Left some comments

modules/yieldlabBidAdapter.js

export const spec = {
code: BIDDER_CODE,
supportedMediaTypes: [VIDEO, BANNER],
Copy link
Collaborator

Choose a reason for hiding this comment

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

banner is default so no need to add in supportedMediaTypes

Copy link
Collaborator

Choose a reason for hiding this comment

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

@GEMI Sorry for the confusion. Can you please add BANNER again.
It will be required once #1991 is merged

modules/yieldlabBidAdapter.js Outdated
ad: `<script src="${ENDPOINT}/d/${matchedBid.id}/${bidRequest.params.accountId}/${sizes[0]}x${sizes[1]}?ts=${timestamp}"></script>`
}
if (isVideo(bidRequest)) {
bidResponse.mediaType = 'video'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you are importing mediaTypes you can use VIDEO const.

modules/yieldlabBidAdapter.md Outdated
var adUnits = [
{
code: "test1",
sizes: [[]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty sizes is not allowed. Prebid core uses this sizes in size mapping modules and also multi format will also use the same.
More about sizes here http://prebid.org/dev-docs/publisher-api-reference.html#module_pbjs.addAdUnits

@GEMI
Copy link
Contributor Author

GEMI commented Jan 23, 2018

I have fixed the points that you have raised. Is the code ok now?

@mkendall07
Copy link
Member

@GEMI
You'll need to add banner media types into the supportedMediaTypes array. Thanks

@GEMI
Copy link
Contributor Author

GEMI commented Jan 24, 2018

Everything in place!

@mkendall07 mkendall07 added this to the Prebid 1.3 release milestone Jan 24, 2018
matthewlane
matthewlane approved these changes Jan 24, 2018
View reviewed changes
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

LGTM. 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

@GEMI GEMI mentioned this pull request Jan 25, 2018
Merged
@GEMI
Copy link
Contributor Author

GEMI commented Jan 25, 2018

I have create a new PR - prebid/prebid.github.io#571

@matthewlane matthewlane merged commit 104376b into prebid:master Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaiminpanchal27 jaiminpanchal27 jaiminpanchal27 approved these changes

@matthewlane matthewlane matthewlane approved these changes

Assignees

@matthewlane matthewlane

Labels
LGTM
Projects
None yet
Milestone
Prebid 1.3 release
Development

Successfully merging this pull request may close these issues.
4 participants
@GEMI @mkendall07 @matthewlane @jaiminpanchal27