-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[WIP] ✨ Add amp-wordpress-embed extension #18412
[WIP] ✨ Add amp-wordpress-embed extension #18412
Conversation
|
||
```html | ||
<amp-wordpress-embed | ||
data-url="https://wordpress.example.com/post-title/" |
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 could update this to be an actual URL, like https://make.wordpress.org/core/2015/10/28/new-embeds-feature-in-wordpress-4-4/
tag_name: "AMP-GIST" | ||
requires_extension: "amp-wordpress-embed" | ||
attrs: { | ||
name: "data-gistid" |
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.
Needs to be updated to data-url
.
} | ||
tags: { # <amp-wordpress-embed> | ||
html_format: AMP | ||
tag_name: "AMP-GIST" |
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.
Needs to be AMP-WORDPRESS-EMBED
<amp-wordpress-embed | ||
layout="fixed-height" | ||
height="241" | ||
data-url="b9bb35bc68df68259af94430f012425f"> |
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.
This should be a URL like https://make.wordpress.org/core/2015/10/28/new-embeds-feature-in-wordpress-4-4/
this.applyFillContent(iframe); | ||
|
||
// Triggered by sendEmbedMessage inside the iframe. | ||
listenFor(iframe, 'height', data => { |
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.
This is triggering an error:
cannot register events on an attached iframe. It will cause hair-pulling bugs like #2942
url.searchParams.set( 'embed', 'true' ); | ||
|
||
const iframe = createFrameFor(this, url.toString()); | ||
this.applyFillContent(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.
This seems redundant because it is done in createFrameFor
.
* <code> | ||
* <amp-wordpress-embed | ||
* layout="fixed-height" | ||
* data-url="https://example.com/2018/05/17/awesome-post/" |
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.
This can be an actual URL like https://make.wordpress.org/core/2015/10/28/new-embeds-feature-in-wordpress-4-4/
586ada2
to
d681aea
Compare
*/ | ||
|
||
/** | ||
* @fileoverview Embeds a Github gist |
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.
Needs to be updated
@@ -0,0 +1,64 @@ | |||
FAIL |
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.
to update the validator .out
files run gulp validator --update_tests
@@ -0,0 +1,40 @@ | |||
# | |||
# Copyright 2017 The AMP HTML Authors. All Rights Reserved. |
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.
Please make sure all license files are the current year 2018
@westonruter is this ready for review? |
@aghassemi no, sorry. We need to pick it up again. Most likely after next week. |
@westonruter is this PR still active? |
It will be. We'll be picking it up again at some point. |
Closing this PR as inactive. Please reopen if needed. |
Fixes #18378.
/cc @amedina