Implement twitter with resize event #3397
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This LGTM thanks for doing some manual testing. @cramforce mind taking a look as well? |
|
|
||
| let twitterWidgetSandbox; | ||
| twttr.events.bind('resize', event => { | ||
| if (twitterWidgetSandbox === event.target) { |
There was a problem hiding this comment.
Maybe add a comment as to what other cases happen here.
|
Nice! So much simpler :) LGTM with one comment. |
| } | ||
| }); | ||
|
|
||
| twttr.widgets.createTweet(data.tweetid, tweet, data).then(el => { |
There was a problem hiding this comment.
Seems you still need to /*OK*/then this promise. You can check for failures locally with gulp presubmit
There was a problem hiding this comment.
Yep. Quick explanation: We do not ship with a promise polyfill inside of this iframe ourselves.
|
Thanks @indianburger for sending this! Let us know if there are some more stuff we can do to make |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Intent to implement reference: #3377
Twitter exposes a resize event which can be used instead of setting up listeners. Also use the element returned by
twttr.widgets.createTweetinstead of looking for iframes/shadow DOM elements. This avoid AMP breaking when twitter changes internal implementation.Testing done:
gulp dist --fortesting; gulp test: I have 2 unrelated tests that fail constantly. Perhaps I'm running it wrongly?Manual tests. Ran the twitter example page in IE edge 12, mobile safari 9.2, Chrome 50, FF 46, Android Chrome 49.
I can do more testing, let me know where I should spend my time.