-
Notifications
You must be signed in to change notification settings - Fork 383
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
Twitter raw embeds are not all converted #4653
Comments
This may be unintentionally fixed by #4650 as I noticed some of the tests in |
The "follow", "hashtag", and "mention" buttons do not seem to be supported by the When the embed is rendered by the accompanied script it produces an iframe: <iframe id="twitter-widget-0" scrolling="no" frameborder="0" allowtransparency="true" allowfullscreen="true" class="twitter-follow-button twitter-follow-button-rendered" style="position: static; visibility: visible; width: 129px; height: 20px;" title="Twitter Follow Button" src="https://platform.twitter.com/widgets/follow_button.c63890edc4243ee77048d507b181eeec.en.html#dnt=true&id=twitter-widget-0&lang=en&screen_name=TwitterDev&show_count=false&show_screen_name=true&size=m&time=1588909307584" data-screen-name="TwitterDev"></iframe> We could attempt to create an |
It looks like there is a feature request to support this: ampproject/amphtml#24678 In the meantime, we'll have to just skip converting such elements. At least the link is a suitable fallback. |
@westonruter We are seeing Twitter not being modified to amp-twitter when the user pastes Tweet URL into the block editor or uses 'Twitter' embed block. My assumption is this is because of lack of a tag or blockquote or potentially a change in WP 5.6. Here is what is stored in post_content <!-- wp:embed {"url":"https://twitter.com/Twitter/status/1360273390070865928","type":"rich","providerNameSlug":"twitter","responsive":true,"className":""} -->
<figure class="wp-block-embed alignwide is-type-rich is-provider-twitter wp-block-embed-twitter"><div class="wp-block-embed__wrapper">
https://twitter.com/Twitter/status/1360273390070865928
</div></figure>
<!-- /wp:embed --> |
@rickalee I'm not seeing that. When I view the AMP version, I get <figure class="wp-block-embed is-type-rich is-provider-twitter wp-block-embed-twitter alignwide">
<div class="wp-block-embed__wrapper">
<amp-twitter width="600" height="480" layout="responsive" data-tweetid="1360273390070865928" class="twitter-tweet i-amphtml-layout-responsive i-amphtml-layout-size-defined" data-width="550" data-dnt="true" i-amphtml-layout="responsive">
<i-amphtml-sizer style="display:block;padding-top:80%"></i-amphtml-sizer>
<blockquote class="twitter-tweet" data-width="550" data-dnt="true" placeholder=""><p lang="en" dir="ltr">
roses are red <br>our avi is blue <br>if you're single<br>we'll reply to you</p>— Twitter (@Twitter)
<a href="https://twitter.com/Twitter/status/1360273390070865928?ref_src=twsrc%5Etfw">February 12,
2021</a></blockquote>
</amp-twitter>
</div>
</figure> |
Bug Description
There are various options for embedding content from Twitter, as can be seen at https://publish.twitter.com/:
Of all these options, only the Tweet currently is properly converted.
This necessitated Jetpack adding special support for embedding Twitter timelines: Automattic/jetpack#15328
This should not have been the case, however: https://github.com/Automattic/jetpack/pull/15328/files#r419632716
Expected Behaviour
Raw embeds code coming from Twitter should be converted into AMP components as required.
Steps to reproduce
Paste this markup while editing code in the block editor:
Screenshots
Validation errors are raised for all embeds other than Tweets:
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation brief
The logic in
AMP_Twitter_Embed_Handler::is_tweet_raw_embed()
is lacking:amp-wp/includes/embeds/class-amp-twitter-embed-handler.php
Lines 132 to 147 in 4880f0f
Namely, in addition to checking for
twitter-tweet
it also needs to account for all the other class names that the embeds utilize, includingtwitter-timeline
,twitter-hashtag-button
,twitter-moment
and so on.QA testing instructions
Demo
Changelog entry
The text was updated successfully, but these errors were encountered: