这是indexloc提供的服务，不要输入任何密码
Skip to content

🚀 [Story embeds] Remove support for expanded components like amp-twitter #36851

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Nov 11, 2021
Merged

🚀 [Story embeds] Remove support for expanded components like amp-twitter #36851

merged 14 commits into from
Nov 11, 2021

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Nov 8, 2021 •
edited
Loading

Closes #36353

Removes the expanded view used in amp-twitter. Uses linking instead (shows tooltip that links to the tweet).
This will allow us to get rid of a significant portion of unused code from amp-story, given that we will not be using the expanded view for other embedded components, and focusing on embedding interactive iframes in page attachments.

Removes 8kB of uncompressed, and 1.4kB compressed

The tweet URL is https://twitter.com/_/status/<tweet_id>. Where the _ goes there should appear the username, but we don't want to make an API request to find that out, so we can "abuse" the fact that tweet_ids are unique and the website redirects to the correct username on load.

Reference: https://blog.twitter.com/developer/en_us/topics/tips/2020/getting-to-the-canonical-url-for-a-tweet

@mszylkowski mszylkowski marked this pull request as ready for review November 10, 2021 19:53
@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/_locales/af.json
extensions/amp-story/1.0/_locales/am.json
extensions/amp-story/1.0/_locales/ar.json
extensions/amp-story/1.0/_locales/bg.json
extensions/amp-story/1.0/_locales/bn.json
extensions/amp-story/1.0/_locales/bs.json
extensions/amp-story/1.0/_locales/ca.json
extensions/amp-story/1.0/_locales/cs.json
extensions/amp-story/1.0/_locales/da.json
extensions/amp-story/1.0/_locales/de.json
extensions/amp-story/1.0/_locales/el.json
extensions/amp-story/1.0/_locales/en-GB.json
+64 more

gmajoulet
gmajoulet approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still things that could be simplified further (e.g. we don't have embedded components anymore, just links) but that's an awesome first step, thank you!

@gmajoulet
Copy link
Contributor

The Intent to deprecate and remove: #36353

Can you test a few real world Stories and make sure their layouts are good and unchanged (cf failing Percy tests)? recipe 538052

@mszylkowski mszylkowski changed the title 🚀 [Story embeds] Remove support for embedded components like amp-twitter [WIP] 🚀 [Story embeds] Remove support for expanded components like amp-twitter [WIP] Nov 11, 2021
@mszylkowski mszylkowski changed the title 🚀 [Story embeds] Remove support for expanded components like amp-twitter [WIP] 🚀 [Story embeds] Remove support for expanded components like amp-twitter Nov 11, 2021
@mszylkowski
Copy link
Contributor Author

we don't have embedded components anymore

We still do, and we need to click shield them + add their tooltips. This PR removes the expanded states though.

Can you test a few real world Stories and make sure their layouts are good and unchanged (cf failing Percy tests)? recipe 538052

Tested the most common stories / domains and they don't break the layout

@mszylkowski
Copy link
Contributor Author

mszylkowski commented Nov 11, 2021 •
edited
Loading

@gmajoulet in more detail, currently interactive (expandable) tweets are re-scaled so they have scale=1 on the expanded view, which means that on the original view they don't meet exactly the layout values they were given and they would shift depending on the page size (making the previous positioning non reponsive). Non-interactive tweets do follow the layout values.

This means by removing this code, we can effectively make interactive and non-interactive tweets consistently behave, and without the weird scaling, the tweets still occupy the same space as before, but now using the scale=1 always (aka, not scaled).

This is preferred and reduces the bundle size significantly, so we're moving forward with the merge.

@mszylkowski mszylkowski merged commit 69b8021 into ampproject:main Nov 11, 2021
@mszylkowski mszylkowski deleted the remove_embedded branch November 11, 2021 21:05
@mszylkowski mszylkowski self-assigned this Nov 19, 2021
@jshamble jshamble mentioned this pull request Nov 29, 2021
Closed
@mszylkowski mszylkowski mentioned this pull request Dec 6, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

Assignees

@mszylkowski mszylkowski

Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[I2D&R] Tweet lightbox within Stories
3 participants
@mszylkowski @gmajoulet @ampprojectbot