AMP integration with Index Exchange #5944
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! |
|
CLAs look good, thanks! |
|
Hey @lannka I took a look at the CI results, there isn't any log outputs, do you happen to know what are the cases that failed? Also I noted that you've been asking companies to implement renderStart and noContentAvailable. IndexExchange is an ad exchange, so we'll always return an ad for each request, so noContentAvailable will not be applicable to us. As for renderStart, we are planning on looking into it on the later update. It is not trivial at this moment to alert AMP when ads starts rendering. Please let me know if you got any other questions, Sincerely, |
|
Looks like the logs were posted late, and I saw the problem. It is due to merge that the alphabetical order were changed. I'll be making an update and submit it again |
|
/to @lannka |
|
Please note this should be covered under the corporate CLA signed with Index Exchange on Oct 24, 2016 11:04 PDT |
There was a problem hiding this comment.
Also please follow https://github.com/ampproject/amphtml/blob/master/ads/README.md#files-to-change here to include change for /extensions/amp-ad/amp-ad.md
| @@ -0,0 +1,26 @@ | |||
| /** | |||
| * Copyright 2015 The AMP HTML Authors. All Rights Reserved. | |||
| * @param {!Window} global | ||
| * @param {!Object} data | ||
| */ | ||
| export function ix(global, data) { |
There was a problem hiding this comment.
I see that you request mandatory data attributes, and accept optional data attributes. I would recommend using the validateData here to check those attributes exist.
https://github.com/ampproject/amphtml/blob/master/3p/3p.js#L215
There was a problem hiding this comment.
Hey Zouyx,
We considered using validataData during development. but we opted not to use it because:
Later updates to our ad server will not require changes within AMP. to remain flexible for future changes. We already validate all parameters on our end.
| @@ -110,6 +110,7 @@ | |||
| <a href="#gmossp" class="broken"></a> | | |||
| <a href="#ibillboard">iBILLBOARD</a> | | |||
| <a href="#imobile">I-Mobile</a> | | |||
| <a href="#ix">Index Exchange</a> | | |||
There was a problem hiding this comment.
please follow Alphabetical order.
|
@indexexchange @IndexExchange-JerryLi @ixpaciga Looks good in general, few comments. |
|
Thanks @zhouyx, I'll make the updates and submit the pull request again. |
|
@IndexExchange-JerryLi Any update here? |
|
Sorry for the delay. We have pushed new changes. Let us know if everything is satisfactory. |
|
also need to change
Another thing is we have a renderStart API that helps AMP to provide better ad loading experience. |
|
We have updated the For noContentAvailable, we always return content for each request, so it is not applicable to us. As for renderStart, we are planning on looking into it on the later update. It is not trivial at this moment to alert AMP when ads starts rendering. |
|
LGTM. ping @lannka to take a final look. |
|
Thanks @IndexExchange-JerryLi for the PR. Merged |
* Add amp-ad support for Index Exchange * fix alphabetical sorting of new ix config * Updated alphabetical order using ix in ads.amp.html and corrected the year of copyright in ix.js. * Add Index Exchange to amp-ad.md file
* Add amp-ad support for Index Exchange * fix alphabetical sorting of new ix config * Updated alphabetical order using ix in ads.amp.html and corrected the year of copyright in ix.js. * Add Index Exchange to amp-ad.md file
Added amp-ad tag of type "ix"