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 embed support for Twitter timelines via new amp-twitter attributes #1396

Merged

Conversation

felixarntz
Copy link
Collaborator

@felixarntz felixarntz commented Sep 4, 2018

amp-twitter now supports arbitrary data attributes for Twitter timelines, based on a required data-timeline-source-type attribute. All other arguments to pass along to the Twitter script must be provided as individual data attributes prefixed with "timeline-". As an example, if you set data-timeline-source-type to "profile", you also need to pass a screenName argument, which would need to happen via data-timeline-screen-name.

This PR adds support for Twitter timelines, by automatically parsing embed URLs into appropriate amp-twitter elements. The code supports profile, likes and list source types.

For reference:

@felixarntz
Copy link
Collaborator Author

When testing the functionality, parsing into the correct tag works, however on the frontend the corresponding iframes only render as blank. I'd need to investigate why this happens, but maybe someone with more AMP experience has a quick idea about it.

@westonruter westonruter self-assigned this Sep 4, 2018
@westonruter westonruter self-requested a review September 4, 2018 17:08
@westonruter
Copy link
Member

@felixarntz Is your testing site in HTTPS? AMP requires HTTPS for amp-iframe to render:

An amp-iframe must only request resources via HTTPS, from a data-URI, or via the srcdoc attribute.

https://www.ampproject.org/docs/reference/components/amp-iframe

* Remove unused function parameters.
* Use named capture groups in regular expressions.
@westonruter westonruter added this to the v1.0 milestone Sep 4, 2018
@westonruter westonruter merged commit 1803e42 into ampproject:develop Sep 4, 2018
@felixarntz
Copy link
Collaborator Author

@westonruter Ah yes, that was it. I should really setup SSL for my VVV sites.

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

Successfully merging this pull request may close these issues.

None yet

2 participants