Skip to content
This repository has been archived by the owner on Apr 2, 2020. It is now read-only.

Add support for Facebook video posts, and for feeds #35

Closed
wants to merge 2 commits into from

Conversation

wpdavis
Copy link

@wpdavis wpdavis commented Jun 30, 2015

Adds support for video posts matching https?://(www)?.facebook.com/[^/]+/videos/[\d]+

Also, moves the Facebook script call inline, else the embed fails in instances where wp_footer is not called (i.e. in feeds)

Adds support for video posts matching https?://(www)?\.facebook\.com/[^/]+/videos/[\d]+

Also, moves the Facebook script call inline, else the embed fails in instances where wp_footer is not called (i.e. in feeds)
@wpdavis
Copy link
Author

wpdavis commented Jun 30, 2015

I know it's dirty to put JS inline. Any ideas on a better way to do this? Maybe something like

if( is_feed() ) { \\do things inline } elseif( ! has_action( 'wp_footer' ) ) { \\enqueue }

Since we removed the facebook connect script call from the footer, also remove the action to call it in the footer.
@danielbachhuber
Copy link
Contributor

Also, moves the Facebook script call inline, else the embed fails in instances where wp_footer is not called (i.e. in feeds)

Why would you want to add the Facebook script to a feed?

@wpdavis
Copy link
Author

wpdavis commented Jul 2, 2015

For us it's specifically because our apps our powered through our feeds. But in general, if there's a JSON response with the content of the post, for example, generally I would think it should include everything needed to render the content. In this case as well there isn't a plain-HTML fallback. Definitely depends on the application, though.

@davisshaver
Copy link
Member

Regarding feeds – we've previously discussed an abstraction that lets us callback a separate feed function to handle this use case. #30 I'm hesitant to place the script calls inline a feed – standards also advise against it

Can we do a new PR with the Facebook videos regex tweak and discuss the context switching here? #38

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants