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

Quantum Advertising Adapter #2051

Merged
merged 9 commits into from
Feb 22, 2018
Merged

Conversation

sami-elasticad
Copy link
Contributor

@sami-elasticad sami-elasticad commented Jan 19, 2018

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
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Initial push for Quantum Advertising Bid Adapter.

  • test parameters for validating bids

Sample Ad Unit: For Publishers

var adUnits = [{
          code: 'quantum-adUnit-id-1',
          sizes: [[300, 250]],
          bids: [{
              bidder: 'quantum',
              params: {
                placementId: 21546 
              }      
            }]
        }];

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer
    sami@elasticad.com
  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@mike-chowla
Copy link
Contributor

There's some commented out code (lines 68 and 74 for example) that should be removed

@mike-chowla
Copy link
Contributor

Code coverage is well below 80%:
screen shot 2018-01-30 at 6 00 15 pm

@mike-chowla
Copy link
Contributor

There's a lot of code in the adapter for native but it the adapter doesn't declare native is a supported media type. If native support is not ready in the adapter, the native code should not be included in the pull request.

@bretg
Copy link
Collaborator

bretg commented Feb 2, 2018

@sami-elasticad - please note the review comments

@mkendall07 mkendall07 added this to the Prebid 1.4.0 milestone Feb 12, 2018
@sami-elasticad
Copy link
Contributor Author

Please review last changes. Thank you.

@mike-chowla
Copy link
Contributor

@sami-elasticad It's good that you've added tests for native and test coverage is now > 80%. But I'm still perplexed as to why there's code to render native but supportedMediaTypes doesn't declare native so the adapter can't actually be used with native.

@sami-elasticad
Copy link
Contributor Author

Hi. I added supportedMediaTypes, it was something i missed. Please review changes. Thank you.

@sami-elasticad
Copy link
Contributor Author

Yet I haven't been able to make a functional test for rendering the native bid. I tried every step from http://prebid.org/dev-docs/show-native-ads.html#how-native-ads-work but i didn't managed to make it working. Can you provide a functional example where a native ad is rendered with prebid.js 1.3+. Thank you.

@mike-chowla
Copy link
Contributor

Here's the test page I use. It's the example from Prebid.org with minor updates for 1.x
pbjs_native.html.zip

@sami-elasticad
Copy link
Contributor Author

Thank you for the test page. I tested and fixed some problems with assets. It worked on my side. Waiting for your reviews.

@mike-chowla
Copy link
Contributor

You should update your docs to include the bannerFormat parameter but otherwise looks good

@mike-chowla mike-chowla merged commit 853a762 into prebid:master Feb 22, 2018
@sami-elasticad
Copy link
Contributor Author

We renounced to bannerFormat param. It works without it, we use mediaTypes only. Thank you.

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.

5 participants