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 support for lazy loading on amp-ad appnexus adapter #24070
✨ Add support for lazy loading on amp-ad appnexus adapter #24070
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
d1fcb63
to
5bf4bb5
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@zhouyx could you please look into this PR. |
CC: @jsnellbaker |
to @ampproject/ads |
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.
Thanks for continued support of this extension! Just a couple of questions.
ads/appnexus.md
Outdated
@@ -65,6 +65,30 @@ Note: you should use either the basic setup or AST setup. Do not mix types on th | |||
</amp-ad> | |||
``` | |||
|
|||
### AST Infinite scroll ads on the page, Include adUnit details only in the amp-ad tag which you want to lazy load | |||
Note: You would loose competitive exclusion if you use this setup. |
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: lose
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.
done, thanks
examples/ads.amp.html
Outdated
</amp-ad> | ||
</amp-ad> | ||
|
||
<amp-ad width="300" height="250" |
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 don't see a creative here, is it possible to render one? It helps us with our future debugging.
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.
We left it to test how would it behave if there was no bid for this tag.
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.
Could you add a comment above the example explaining. 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.
done, thanks
examples/ads.amp.html
Outdated
|
||
<amp-ad width="728" height="90" | ||
<amp-ad width="728" height="90" |
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: can you make indentation consistent across the three examples here?
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.
done, 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.
LGTM thanks for contributing! Looks like a percy flake. I restarted the test. Ill try to keep an eye on it but let me know if you see it pass
Looks like all the required checks are cleared. Thanks |
Merged! Thanks again. |
Thank you, when can we expect these changes to go live. |
This should be released in the new canary tomorrow, and production the following week. For more details please check out the release schedule documentation. Let me know if you have any questions. |
…#24070) * appnexus Add support for multiple loadTags * Update appnexus ad example * Add docs for amp-ads appnexus lazyload * resize example ad appnexus tag * update docs appnexus ads * change Array includes to indexOf * review changes * code review changes
This pull req adds the ability to delay the request of ads on infinite scroll pages.