-
Notifications
You must be signed in to change notification settings - Fork 123
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
Adds Facebook App ID filter #585
base: master
Are you sure you want to change the base?
Adds Facebook App ID filter #585
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Code looks good, just wanna get this tested and then we can roll it in to the next release.
@@ -7,6 +7,9 @@ | |||
$share_link = $this->get( 'share_link' ); | |||
$update_time = $this->get( 'update_time' ); | |||
$share_link_amp = $this->get( 'share_link_amp' ); | |||
|
|||
/* This filter is defined in class-wpcom-liveblog-amp.php */ | |||
$facebook_app_id = apply_filters( 'liveblog_amp_facebook_share_app_id', false ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering how this is going to be used, it feels like a default value of ''
empty string, would be in keeping the right type at least.
Would be good to get this filter documented on how to use it too. |
The
LIVEBLOG_AMP_FACEBOOK_SHARE
constant is undefined by the plugin, undocumented, and were it to be used, would be broken due to #584.This change defines a filter
liveblog_amp_facebook_share_app_id
which enables themes to set the Facebook App ID enabling sharing to Facebook.Fixes #584