-
Notifications
You must be signed in to change notification settings - Fork 384
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
Update amp-twitter to use fixed-height layout with a default height matching minimal tweet height #6504
Conversation
…atching minimal tweet height
Plugin builds for 4700260 are ready 🛎️!
|
@@ -161,9 +181,9 @@ private function create_amp_twitter_and_replace_node( Document $dom, DOMElement | |||
} | |||
|
|||
$attributes = [ | |||
'width' => $this->DEFAULT_WIDTH, | |||
'width' => 'auto', |
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.
Any reason for this change?
'width' => 'auto', | |
'width' => $this->DEFAULT_WIDTH, |
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.
Yes, because auto
is the only valid width for the fixed-height
layout.
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.
Oh wait, you're right. I've revised in #6510
QA In Progress Question: how should this be QA'ed? Should Bento be enabled and if yes, then which sandboxing level should it be on? Should I use something like this in a Custom HTML block: <script type="module" src="https://cdn.ampproject.org/bento.mjs" crossorigin="anonymous"></script>
<script nomodule="" src="https://cdn.ampproject.org/bento.js" crossorigin="anonymous"></script>
<script type="module" src="https://cdn.ampproject.org/v0/bento-twitter-1.0.mjs" crossorigin="anonymous"></script>
<script nomodule="" src="https://cdn.ampproject.org/v0/bento-twitter-1.0.js" crossorigin="anonymous"></script>
<link rel="stylesheet" href="https://cdn.ampproject.org/v0/bento-twitter-1.0.css" crossorigin="anonymous">
<bento-twitter width="auto" height="197" layout="fixed-height" id="my-tweet-1" data-tweetid="1410342522820300802"></bento-twitter> Or should I just embed a Tweet in the post and let WordPress convert it to an embed? In the latter case, AMP converts the embed to I think that answers to those questions may be important for #6502 and it might need to be QA'ed again. |
@delawski Yes, using However, to test this properly you'd want to embed a Tweet using the Tweet embed block. Even if you're in the Loose level for Sandboxing, this alone wouldn't trigger the Bento version since it's a valid AMP page and we currently only opt for Bento versions if there is something on the page that is downgrading it to Moderate or Loose, since Bento components are especially important for encapsulation. So an easy way to force the Moderate level is to just add a Custom HTML block that has this: <script data-px-verified-tag>console.info('PX-verified!');</script> I just checked and in Level 3 with non-Bento component, the markup is: <amp-twitter width="auto" height="197" layout="fixed-height" data-tweetid="1410342522820300802" class="twitter-tweet i-amphtml-layout-fixed-height i-amphtml-layout-size-defined" data-width="550" data-dnt="true" style="height:197px" i-amphtml-layout="fixed-height"><button overflow type="button">See more</button><blockquote class="twitter-tweet" data-width="550" data-dnt="true" placeholder=""><p lang="en" dir="ltr">Rideshare deployment sequence complete</p>— SpaceX (@SpaceX) <a href="https://twitter.com/SpaceX/status/1410342522820300802?ref_src=twsrc%5Etfw">June 30, 2021</a></blockquote></amp-twitter> And with in Level 3 with the Bento component the markup is (also): <amp-twitter width="auto" height="197" layout="fixed-height" data-tweetid="1410342522820300802" class="twitter-tweet i-amphtml-layout-fixed-height i-amphtml-layout-size-defined" data-width="550" data-dnt="true" style="height:197px" i-amphtml-layout="fixed-height"><button overflow type="button">See more</button><blockquote class="twitter-tweet" data-width="550" data-dnt="true" placeholder=""><p lang="en" dir="ltr">Rideshare deployment sequence complete</p>— SpaceX (@SpaceX) <a href="https://twitter.com/SpaceX/status/1410342522820300802?ref_src=twsrc%5Etfw">June 30, 2021</a></blockquote></amp-twitter> The difference is in the rendering:
In non-Bento, there is a layout shift and the entire Tweet is shown. In Bento, there is no layout shift and an overflow button is shown. |
QA Passed ✅ After following @westonruter's instructions (thank you for that 🙌), I was able to confirm that Bento version of
|
Note also that |
Summary
This is a follow-up on #6502.
When the Bento version of
amp-twitter
is embedded on a page, it does not resize automatically when in the initial viewport. Since the default dimensions of an embedded Tweet was 600x480 with responsive layout, the result is an element with a lot of bottom padding. And since the element is taller than the contents require, AMP does not show theoverflow
button since the element needs to shrink not grow. Therefore, I've updated the default height to match the smallest Tweet I could find. This will ensure that the Tweet will only ever grow in height, and not need to shrink. Also, I changed thelayout
fromresponsive
tofixed-height
which seems more appropriate, as Tweets generally do have a fixed height.Checklist