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

Add New Adapter dgadsBidAdapter #2429

Merged
merged 12 commits into from
Apr 24, 2018
Merged

Conversation

r-sato
Copy link
Contributor

@r-sato r-sato commented Apr 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

  • test parameters for validating bids
{
  bidder: 'dgads',
  params: {
    location_id: '1',
    site_id: '1';
}

@jsnellbaker jsnellbaker self-assigned this Apr 20, 2018
@jsnellbaker jsnellbaker self-requested a review April 20, 2018 15:54
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hello @r-sato thanks for submitting this new adapter.

Overall the code looks to be good, except for a few things. See the areas below for the code-specific feedback.

In terms of the testing, I was having some trouble getting the test banner unit to render on the hello_world.html page. The bidResponse seemed fine but the rendering just didn't happen. Was this working for you on the hello_world page?

Please let me know when you get the chance.

mediaTypes: 'banner',
params: {
location_id: '1',
site_id: '1';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this semi-colon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. We corresponded.

bidResponse.netRevenue = true;
bidResponse.ttl = ad.ttl;
bidResponse.referrer = utils.getTopWindowUrl();
if (ad.isNative == 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhere in here I would recommend to set the bid's mediaType to native.

When I was testing the native ad with the .md file's test params, the bid's mediaType was set to the default banner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. We corresponded.

@r-sato
Copy link
Contributor Author

r-sato commented Apr 21, 2018

Hi @jsnellbaker, Thank you for the review.
We fixed both and committed.

And regarding hello_world.html,
the image server was down, so we responded.
Please check again.

@jsnellbaker
Copy link
Collaborator

Hi @r-sato, thanks for making the changes.

I retested and found the banner creative was showing successfully. However the native test ad wasn't rendering. When I was looking at the browser console to debug the issue, I realized that the pbjs.renderAd function was being invoked - which is normally used for banner or video ads. Native creatives use a different template to render themselves given the variety of creative assets that may/may not be used.

The following page gives an outline of what the native creative code looks like: http://prebid.org/adops/setting-up-prebid-native-in-dfp.html Can you take a look over this page?

@r-sato
Copy link
Contributor Author

r-sato commented Apr 24, 2018

Hi @jsnellbaker, thank you for the check.

Our native ad unit is rendered successfully.
We have registered HTML and CSS with reference to http://prebid.org/adops/setting-up-prebid-native-in-dfp.html.

This is our test html for native.
hello_world_native

Please let me know if anything differs.

@jsnellbaker
Copy link
Collaborator

@r-sato Thanks for making the updates and providing the sample test page. Everything looks good now.

@jsnellbaker jsnellbaker merged commit 372899b into prebid:master Apr 24, 2018
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Add dgads adapter

* Add dgads adapter

* Add spec file for dgads

* Add spec file for dgads

* Add dgads bid adapter

* Add dgads bid adapter

* fix

* fix

* fix email

* remove  semi-colon

* Add mediaType native

* Add import medaTypes
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.

2 participants