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
Adding a new ad network - AdPicker #13171
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! (Google group name: hitokuse) |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Few comments.
ads/adpicker.js
Outdated
validateData(data, ['ph']); | ||
const url = 'https://cdn.adpicker.net' + | ||
'/ads/main.js?et=amp' + | ||
'&ph=' + data.ph + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: encodeURIComponent(data.ph)
?
ads/adpicker.md
Outdated
Supported parameters: | ||
|
||
**Required** | ||
- width: required by amp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: It is not always the case that AMP require both width
and height
. Please refer to supported amp-ad doc here.
Please only specify parameters required by your ad network here. If you want to add instructions on how to use the <amp-ad>
, you can refer to our README here.
examples/ads.amp.html
Outdated
@@ -349,6 +350,12 @@ <h2>AdOcean</h2> | |||
> | |||
</amp-ad> | |||
|
|||
<h2>AdPicker</h2> | |||
<amp-ad width="300" height="250" | |||
type="adpicker" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation.
@@ -349,6 +350,12 @@ <h2>AdOcean</h2> | |||
> | |||
</amp-ad> | |||
|
|||
<h2>AdPicker</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test with your ad. But it returns a no content ad to me. Could you provide a fake/demo ad example here for us to test. Thanks.
Thank you so much for your review @zhouyx !! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shun0750 Looks good. I have one final comment. Please let me know if I should merge this PR. Thanks.
ads/adpicker.md
Outdated
Supported parameters: | ||
|
||
**Required** | ||
- width: width of ad area |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment before
FYI: It is not always the case that AMP require both width and height. Please refer to supported amp-ad doc here.
Is there a reason that you require width
and height
to be set? Or is there a reason that you want to document them here? Otherwise we would generally not advice you to document these two attribute here because they are not specific to your adpicker ad network.
I'll leave the final decision to you.
@zhouyx Thank you so much for your review! I removed width/height requirements according to your advice. |
@Shun0750 Thanks for the Pull Request! Merged! |
We added our ad network "AdPicker".
Could you review our commits?