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
Improve fake ad network by providing working example, updating doc, a… #9295
Changes from all commits
70f5523
4a2a344
50e39d3
1624255
be45a36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<!doctype html> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually, would you move this file to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do customers know to look there? What is the intended difference between an example and a manual test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My preference would be to leave it in examples/ so that an AMP page author that wants to write a page and test including ads in it can use this. But it's up to you. Do you think this is valid/useful? |
||
<html ⚡> | ||
<head> | ||
<meta charset="utf-8"> | ||
<title>Fake Ad Network Test</title> | ||
<link rel="canonical" href="amps.html" > | ||
<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
<link href='https://fonts.googleapis.com/css?family=Georgia|Open+Sans|Roboto' rel='stylesheet' type='text/css'> | ||
<script async custom-element="amp-ad" src="https://cdn.ampproject.org/v0/amp-ad-0.1.js"></script> | ||
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> | ||
<script async src="https://cdn.ampproject.org/v0.js"></script> | ||
</head> | ||
<body> | ||
<h2>Fake Ad Network:</h2> | ||
<amp-ad width="300" height="400" | ||
type="fake" | ||
src="fake_amp.json" | ||
data-use-a4a="true"> | ||
<div placeholder>Loading...</div> | ||
<div fallback>Could not display the fake ad :(</div> | ||
</amp-ad> | ||
<br/> | ||
</body> | ||
</html> |
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.
(Copying comment over from outdated code, with slight edits for clarity)
@lannka Re: your previous question, it turns out that the problem was here. "unwrap" was implemented backwards. If the creative is a .json file, then it should be unwrapped (signature and HTML extracted), and if it's a .html file then that is not needed (and the extension should NOT be stripped off). But "unwrap" was done the opposite way. My other main change here is the line that sets the X-AmpAdRender header, which removes the need for the change to amp-a4a.js which you weren't a fan of. Now,
amphtml/extensions/amp-a4a/0.1/amp-a4a.js
Line 611 in 49951ce
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.
much better. appreciate that.