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
amp-iframe guideline to prefer existing components #7264
Conversation
extensions/amp-iframe/amp-iframe.md
Outdated
`amp-iframe` should be considered a fallback if the required user experience is not possible by other means in AMP. This is because there are many benefits to using a component tailored for a specific use-case instead, such as | ||
|
||
- Better resource management and performance | ||
- Increased loading priority (`amp-iframe` loads last, because AMP doesn't know what's inside of it) |
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.
Not correct
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.
Got it—is there anything we can say around better loading priority for specific components v. amp-iframe, or should this line just be deleted?
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.
Just delete it.
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
extensions/amp-iframe/amp-iframe.md
Outdated
- Better resource management and performance | ||
- Increased loading priority (`amp-iframe` loads last, because AMP doesn't know what's inside of it) | ||
- Custom components can provide built-in placeholder images in some cases. This means getting, say, the right video thumbnail before a video loads, and reduces the coding effort to add a placeholder manually. | ||
- Built-in resize requests. This means that iframe content with unpredictable size can more often appear to the user as if it were native to the page, rather than in a scrollable frame |
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.
Built-in resizing.
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
extensions/amp-iframe/amp-iframe.md
Outdated
@@ -208,3 +208,13 @@ Iframes are identified as tracking/analytics iframes if they appear to serve no | |||
## Validation | |||
|
|||
See [amp-iframe rules](https://github.com/ampproject/amphtml/blob/master/extensions/amp-iframe/0.1/validator-amp-iframe.protoascii) in the AMP validator specification. | |||
|
|||
## Guideline: prefer existing AMP components to `amp-iframe` |
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.
s/existing/specific
Please add link to list of components.
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 for the feedback—should be updated to address the comments now |
@@ -208,3 +208,12 @@ Iframes are identified as tracking/analytics iframes if they appear to serve no | |||
## Validation | |||
|
|||
See [amp-iframe rules](https://github.com/ampproject/amphtml/blob/master/extensions/amp-iframe/0.1/validator-amp-iframe.protoascii) in the AMP validator specification. | |||
|
|||
## Guideline: prefer [specific AMP components](https://github.com/ampproject/amphtml/tree/master/extensions) to `amp-iframe` |
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.
When this goes into the docs site, please check that the link there points to the right destination. CC @pbakaus
Thought it would be good to document this somewhere, but want to make sure it's accurate and useful. Not sure who is the best person to review, but /to @cramforce as a first guess, given the component history