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

Add amp-facebook element for Facebook posts and videos #1479

Merged
merged 1 commit into from
Jan 20, 2016

Conversation

mkhatib
Copy link
Contributor

@mkhatib mkhatib commented Jan 20, 2016

I posted some of the decisions I made here in #979 but for ease of access, re-posting here:

I went the amp-twitter approach few exceptions. One is that the only way I got FB SDK to render posts correctly when there are multiple amp-facebook elements on the page is to actually load the SDK script in every amp-facebook iframe.

Before that, following amp-twitter approach and only loading it once FB would only render the post that included the SDK correctly and the rest failed to fetch or render at all until I started to manually calling parse FB.XFBML.parse(parentElement) which would successfully fetch the posts but fails to render the post with the correct height and width and instead renders a 0px by 0px posts and throws an error fb-post failed to resize in 45s.

As I mentioned the only way I got this to work and render sizes correctly is by including the sdk and calling FB.init in every amp-facebook iframe.

Another issue that I bumped into early on was the inability to access contentWindow for the FB post iframe to listen to resize event, I am not entirely sure but I think that's the expected behavior for iframes that are loaded from a different domain (In comparison, twitter's iframe doesn't actually have a src).

Luckily FB SDK has an alternative for contentWindow.resize. FB.Event.subscribe allows us to listen to xfbml.resize event that the SDK fires which also sends the height and width of the resize event - I use that to fire the embed-size event to allow the parent iframe to resize itself. The only concern I have here is that FB doesn't seem to document xfbml.resize event anywhere in their documentation, I had to do some debugging to see what kind of events FB is firing. So not sure if this is a safe approach for now.

Over all, FB SDK lack of a twitter-like programatic way to create posts createTweet was a bit painful. And I am hoping once we get FB devs input on this we can improve the implementation by removing the multiple inclusions of the SDK.

});


function render(width, height) {
Copy link
Member

Choose a reason for hiding this comment

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

Probably not a good name anymore, since this does not re-render. Might be best to just inline the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlined.

@cramforce
Copy link
Member

Looks great. I think sharing the script is not as crucial as with tweets as it is not so common to have many of these posts on the same page (I think).

// https://github.com/ampproject/amphtml/issues/728
// is being implemented.

if (['facebook', 'twitter'].indexOf(data.type) !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer something stupid like two comparisons with a || in between :)

@cramforce
Copy link
Member

Looks great. Some style nits. Will merge after fixing.

@mkhatib
Copy link
Contributor Author

mkhatib commented Jan 20, 2016

Fixed and squashed! Thanks for the review!

@cramforce
Copy link
Member

Thanks!

LGTM

@cramforce cramforce added the LGTM label Jan 20, 2016
cramforce added a commit that referenced this pull request Jan 20, 2016
Add amp-facebook element for Facebook posts and videos
@cramforce cramforce merged commit 52fb826 into ampproject:master Jan 20, 2016
@mkhatib mkhatib deleted the amp-facebook branch January 20, 2016 05:18
@henel677 henel677 mentioned this pull request Mar 3, 2021
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.

2 participants