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

🐛 Fix amp-twitter component resizing issue #37740

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

deepaklalwani97
Copy link
Contributor

Fixes #37117

This PR fixes the Twitter embed resizing issue using the workaround mentioned by @alanorozco in #37117 (comment). As the resizing of the Twitter embed happens only when it comes into viewport in most cases the embed gets loaded in the unexpanded state this PR uses a workaround to resize the embed when the component gets built.

@deepaklalwani97 deepaklalwani97 marked this pull request as ready for review February 22, 2022 07:08
@dethstrobe
Copy link
Contributor

I'm going to look at making an e2e test for this. I'm actually not clear how feasible that it is to test this interaction, but I think it'll be worth a shot to avoid regressions.

@dethstrobe
Copy link
Contributor

Well after some quick-ish investigation it appears that the bento-twitter web-component is broken and never had any e2e tests build for it. But I don't think this is a blocker for this PR, but it in an obvious over sight that we'll need to fix later.

@dmanek
Copy link
Contributor

dmanek commented Feb 25, 2022

Thanks for the fix @deepaklalwani97. I'm approving this, but also adding @alanorozco to approve.

@dmanek dmanek requested a review from alanorozco February 25, 2022 17:24

if (iframeRef.current.node.getRootNode().host) {
// Remove the position style from embed after the resize is complete.
resetStyles(iframeRef.current.node.getRootNode().host, [
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a classname with JSS instead of using setStyles/resetStyles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @alanorozco I have replaced the setStyles/resetStyles with JSS classes

@dethstrobe dethstrobe enabled auto-merge (squash) March 14, 2022 14:50
auto-merge was automatically disabled March 15, 2022 06:51

Head branch was pushed to by a user without write access

@deepaklalwani97
Copy link
Contributor Author

Hey, @dethstrobe I have fixed some merge conflicts on this PR which seems to have disabled the auto-merge. Please let me know if I can help unblock this PR in any way.

@stale
Copy link

stale bot commented Apr 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Apr 2, 2023
@stale stale bot removed the Stale Inactive for one year or more label May 2, 2023
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.

Twitter bento component does not resize properly
5 participants