request for integrating adman ad-server #2744
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! |
|
@dvoytenko I have closed the previous PR #2411 due to wrong merging flow. |
| * @param {!Object} data | ||
| */ | ||
| export function adman(global, data) { | ||
| const script = document.createElement('script'); |
There was a problem hiding this comment.
To confirm, are any of these data parameters required? If so, you may want to check them using validateDataExists.
Also, ideally you'd check the allowed set of data parameters using checkData. This would help your customers to avoid issues with misspellings, etc.
There was a problem hiding this comment.
explicitly use global.document please
|
@dvoytenko I've followed your suggestions and pushed an update |
| const fields = ['ws', 'host', 's']; | ||
|
|
||
| checkData(data, fields); | ||
| validateDataExists(data, fields); |
There was a problem hiding this comment.
@anmarkom Just to confirm, all three data parameters are required, right? If so, could you please make a note about this in the adman.md docs.
|
@anmarkom Looks great. Just one more documentation request. |
|
@dvoytenko I've updated the documentation |
|
Thanks @anmarkom! All looks good. The only thing is that it's now out of sync with master. Could you please rebase? I'll be able to merge then. |
| @@ -0,0 +1,38 @@ | |||
| /** | |||
| * Copyright 2015 The AMP HTML Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
2016 here and every other new file please
anmarkom
force-pushed
the
adman
branch
2 times, most recently
from
1896383
to
913f04c
Compare
|
@dvoytenko I've changed the copyright dates and rebased. Thank you too! |
|
@anmarkom Not sure what happened with rebase or it might be a new conflict. Also, in JS file you call |
|
I have signed the CLA on behalf of Phaistos Networks. |
|
@anmarkom tests look good now. But still couple of issues:
|
|
I have resubmitted the CLA, as it seems that last time it was rejected. On git log my email was already set to my corp mail, however I changed my primary email on GitHub in case it was needed. |
|
@anmarkom Sounds good. This is just now needs to be rebased to the newest master. It looks like another conflict squeaked in. |
add adman to builtins/amp-ad.md and check for required attributes update adman.md documentation adman.js use global
|
CLAs look good, thanks! |
|
Merged! Thanks a lot. |
This PR provides support for ADMAN, the most widely used Ad Server in Greece. It is also used by publishers, advertisers and agencies in various other countries and serves many dozens of billions impressions per month.