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 warning in validator about the deprecation of using amp-ad without loading the extension #4140

Closed
jasti opened this issue Jul 20, 2016 · 6 comments

Comments

@jasti
Copy link
Contributor

jasti commented Jul 20, 2016

Is <script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js"></script> required for running ads? I didn't think so because ads just work without it? e.g. http://dfp-amp-testing-1185.appspot.com/amp_tests/dfp-image-layouts-fixed-demo.html

If not, should we update the documentation here ? https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad/amp-ad.md#-amp-ad

@zhouyx
Copy link
Contributor

zhouyx commented Jul 20, 2016

It's not really required, but like all other extensions it would be good to include script to the page.
I think we deliberately add that require for script line to the documentation to encourage users including script in future. @cramforce

@jasti
Copy link
Contributor Author

jasti commented Jul 20, 2016

Thanks @zhouyx! I think it was throwing off some folks as to how ads were working without the ads script :) I'll send a tiny PR to clarify here.

@Gregable
Copy link
Member

Initially the ad code was part of the main runtime, but was moved to a separate include. Ideally documents would add the include directly, which would speed up loading, however we don't want to break everyone.

Maybe we can leave this issue open as a TODO for the validator to emit warnings for pages that use <amp-ad>, but do not include amp-ad-0.1.js

@jasti
Copy link
Contributor Author

jasti commented Jul 20, 2016

Cool! I've clarified in a PR for doc. Assigning to you.

@jasti jasti assigned Gregable and unassigned lannka Jul 20, 2016
@jasti jasti added this to the Current milestone Jul 20, 2016
@lannka lannka changed the title amp-ad need additional script? Document & add warning in validator about the deprecation of using amp-ad without loading the extension Jul 28, 2016
@Gregable
Copy link
Member

Gregable commented Aug 1, 2016

I'd like to leave this open to add validation warnings about the same. I'll change the subject to no longer mention documentation as that's now in place.

@Gregable Gregable reopened this Aug 1, 2016
@Gregable Gregable changed the title Document & add warning in validator about the deprecation of using amp-ad without loading the extension Add warning in validator about the deprecation of using amp-ad without loading the extension Aug 1, 2016
@rudygalfi rudygalfi modified the milestones: Next, Current Aug 11, 2016
@Gregable
Copy link
Member

Warning should be live in a couple hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants