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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Nov 11, 2021

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Nov 8, 2021

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

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 gmajoulet added this to In progress in wg-stories Sprint via automation Nov 10, 2021
@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

@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
wg-stories Sprint automation moved this from In progress to Done Nov 11, 2021
@mszylkowski mszylkowski deleted the remove_embedded branch November 11, 2021 21:05
@mszylkowski mszylkowski self-assigned this Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[I2D&R] Tweet lightbox within Stories
3 participants