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

Replace the Facebook Like Box plugin with the Facebook Page Plugin (#1992) #2237

Merged
merged 7 commits into from Jun 17, 2015

Conversation

Projects
None yet
5 participants
@justinshreve
Contributor

justinshreve commented Jun 10, 2015

Fixes #1992.

Facebook is deprecating the Facebook Like Box on June 23rd (see https://developers.facebook.com/docs/plugins/like-box-for-pages).

The only option to keep supporting this popular widget is to upgrade to the Facebook page plugin (see https://developers.facebook.com/docs/plugins/page-plugin). Facebook recently made it so that the page plugin can be displayed down to widths of 180 which means many many more themes can support the widget.

This PR does the upgrade, removes settings that no longer work (like color scheme) and add an option for showing stream posts.

Apparently If you do not manually upgrade the Page Plugin (which this PR does), the Like Box plugin implementation will automatically fall back to the Page Plugin on June 23rd 2015. Assuming this is correct, if we can get this merged in for 2.6 we should be OK - but if there is an earlier point release that could be good too.

I'll implement these changes on the WordPress.com side as well.

Testing Steps:

  • Prior to testing this branch, create a Facebook like box widget and customize some of the settings
  • Switch to this branch
  • See if your settings persisted across to the new widget
  • Change a few of the settings (such as displaying cover photo or not, height, etc)
  • Check the front end to make sure your settings change
Replace the Facebook Like Box plugin with the Facebook Page Plugin. T…
…he like box is being deprecated at the end of June, but the page plugin offers the same functions.
Add an overflow:hidden; to the widget so if a sidebar is less than 18…
…0px (such as a resized by browser twentyfourteen theme) it doesn't hang off of the sidebar.
@justinshreve

This comment has been minimized.

Show comment
Hide comment
@justinshreve

justinshreve Jun 10, 2015

Contributor

Per some feedback from the theme team, I've updated the PR to add an overflow: hidden; on the widget. Some responsive themes might make a sidebar slightly less than 180 when resizing the browser, and this prevents an ugly overhang.

Contributor

justinshreve commented Jun 10, 2015

Per some feedback from the theme team, I've updated the PR to add an overflow: hidden; on the widget. Some responsive themes might make a sidebar slightly less than 180 when resizing the browser, and this prevents an ugly overhang.

@justinshreve

This comment has been minimized.

Show comment
Hide comment
@justinshreve

justinshreve Jun 11, 2015

Contributor

Made another change to just remove the width attribute. Without it, the page plugin dynamically figures stuff out.

Contributor

justinshreve commented Jun 11, 2015

Made another change to just remove the width attribute. Without it, the page plugin dynamically figures stuff out.

@justinshreve

This comment has been minimized.

Show comment
Hide comment
@justinshreve

justinshreve Jun 11, 2015

Contributor

These changes are now live on WordPress.com.

Contributor

justinshreve commented Jun 11, 2015

These changes are now live on WordPress.com.

@justinshreve

This comment has been minimized.

Show comment
Hide comment
@justinshreve

justinshreve Jun 16, 2015

Contributor

Just a quick update that I've had no reports of any problems on .com with these changes.

@georgestephanis when is 3.5.4 slated to be released (I see you tagged this release)? Do we think its before the 23rd?

Contributor

justinshreve commented Jun 16, 2015

Just a quick update that I've had no reports of any problems on .com with these changes.

@georgestephanis when is 3.5.4 slated to be released (I see you tagged this release)? Do we think its before the 23rd?

@@ -42,6 +37,8 @@ function widget( $args, $instance ) {
$like_args = $this->normalize_facebook_args( $instance['like_args'] );
wp_enqueue_style( 'jetpack_facebook_likebox', plugins_url( 'facebook-likebox/style.css', __FILE__ ) );

This comment has been minimized.

@georgestephanis

georgestephanis Jun 16, 2015

Member

This feels kinda silly for a three line css file.

Will it work properly if we add in a wp_style_add_data( 'jetpack_facebook_likebox', 'jetpack-inline', true ); to it to trigger Jetpack::maybe_inline_style() ?

jetpack/class.jetpack.php

Lines 5320 to 5382 in 990783b

/**
* Maybe inlines a stylesheet.
*
* If you'd like to inline a stylesheet instead of printing a link to it,
* wp_style_add_data( 'handle', 'jetpack-inline', true );
*
* Attached to `style_loader_tag` filter.
*
* @param string $tag The tag that would link to the external asset.
* @param string $handle The registered handle of the script in question.
*
* @return string
*/
public static function maybe_inline_style( $tag, $handle ) {
global $wp_styles;
$item = $wp_styles->registered[ $handle ];
if ( ! isset( $item->extra['jetpack-inline'] ) || ! $item->extra['jetpack-inline'] ) {
return $tag;
}
if ( preg_match( '# href=\'([^\']+)\' #i', $tag, $matches ) ) {
$href = $matches[1];
// Strip off query string
if ( $pos = strpos( $href, '?' ) ) {
$href = substr( $href, 0, $pos );
}
// Strip off fragment
if ( $pos = strpos( $href, '#' ) ) {
$href = substr( $href, 0, $pos );
}
} else {
return $tag;
}
$plugins_dir = plugin_dir_url( JETPACK__PLUGIN_FILE );
if ( $plugins_dir !== substr( $href, 0, strlen( $plugins_dir ) ) ) {
return $tag;
}
// If this stylesheet has a RTL version, and the RTL version replaces normal...
if ( isset( $item->extra['rtl'] ) && 'replace' === $item->extra['rtl'] && is_rtl() ) {
// And this isn't the pass that actually deals with the RTL version...
if ( false === strpos( $tag, " id='$handle-rtl-css' " ) ) {
// Short out, as the RTL version will deal with it in a moment.
return $tag;
}
}
$file = JETPACK__PLUGIN_DIR . substr( $href, strlen( $plugins_dir ) );
$css = Jetpack::absolutize_css_urls( file_get_contents( $file ), $href );
if ( $css ) {
$tag = "<!-- Inline {$item->handle} -->\r\n";
if ( empty( $item->extra['after'] ) ) {
wp_add_inline_style( $handle, $css );
} else {
array_unshift( $item->extra['after'], $css );
wp_style_add_data( $handle, 'after', $item->extra['after'] );
}
}
return $tag;
}

@georgestephanis

georgestephanis Jun 16, 2015

Member

This feels kinda silly for a three line css file.

Will it work properly if we add in a wp_style_add_data( 'jetpack_facebook_likebox', 'jetpack-inline', true ); to it to trigger Jetpack::maybe_inline_style() ?

jetpack/class.jetpack.php

Lines 5320 to 5382 in 990783b

/**
* Maybe inlines a stylesheet.
*
* If you'd like to inline a stylesheet instead of printing a link to it,
* wp_style_add_data( 'handle', 'jetpack-inline', true );
*
* Attached to `style_loader_tag` filter.
*
* @param string $tag The tag that would link to the external asset.
* @param string $handle The registered handle of the script in question.
*
* @return string
*/
public static function maybe_inline_style( $tag, $handle ) {
global $wp_styles;
$item = $wp_styles->registered[ $handle ];
if ( ! isset( $item->extra['jetpack-inline'] ) || ! $item->extra['jetpack-inline'] ) {
return $tag;
}
if ( preg_match( '# href=\'([^\']+)\' #i', $tag, $matches ) ) {
$href = $matches[1];
// Strip off query string
if ( $pos = strpos( $href, '?' ) ) {
$href = substr( $href, 0, $pos );
}
// Strip off fragment
if ( $pos = strpos( $href, '#' ) ) {
$href = substr( $href, 0, $pos );
}
} else {
return $tag;
}
$plugins_dir = plugin_dir_url( JETPACK__PLUGIN_FILE );
if ( $plugins_dir !== substr( $href, 0, strlen( $plugins_dir ) ) ) {
return $tag;
}
// If this stylesheet has a RTL version, and the RTL version replaces normal...
if ( isset( $item->extra['rtl'] ) && 'replace' === $item->extra['rtl'] && is_rtl() ) {
// And this isn't the pass that actually deals with the RTL version...
if ( false === strpos( $tag, " id='$handle-rtl-css' " ) ) {
// Short out, as the RTL version will deal with it in a moment.
return $tag;
}
}
$file = JETPACK__PLUGIN_DIR . substr( $href, strlen( $plugins_dir ) );
$css = Jetpack::absolutize_css_urls( file_get_contents( $file ), $href );
if ( $css ) {
$tag = "<!-- Inline {$item->handle} -->\r\n";
if ( empty( $item->extra['after'] ) ) {
wp_add_inline_style( $handle, $css );
} else {
array_unshift( $item->extra['after'], $css );
wp_style_add_data( $handle, 'after', $item->extra['after'] );
}
}
return $tag;
}

This comment has been minimized.

@georgestephanis

georgestephanis Jun 16, 2015

Member

Alternately, we can just roll it into our concatenated stylesheet and load it in there. Either way.

@georgestephanis

georgestephanis Jun 16, 2015

Member

Alternately, we can just roll it into our concatenated stylesheet and load it in there. Either way.

This comment has been minimized.

@justinshreve

justinshreve Jun 16, 2015

Contributor

Yeah, on WordPress.com the file is concatenated so an "extra" file is preferred to the inline style. I wasn't aware of maybe_inline_style. I'll take a look.

@justinshreve

justinshreve Jun 16, 2015

Contributor

Yeah, on WordPress.com the file is concatenated so an "extra" file is preferred to the inline style. I wasn't aware of maybe_inline_style. I'll take a look.

This comment has been minimized.

@justinshreve

justinshreve Jun 16, 2015

Contributor

maybe_inline_style works for this. Added it c191a22

@justinshreve

justinshreve Jun 16, 2015

Contributor

maybe_inline_style works for this. Added it c191a22

@dereksmart

This comment has been minimized.

Show comment
Hide comment
@dereksmart

dereksmart Jun 16, 2015

Member

@justinshreve are the console warnings unavoidable?

https://cloudup.com/c9eT-o-8HED

Member

dereksmart commented Jun 16, 2015

@justinshreve are the console warnings unavoidable?

https://cloudup.com/c9eT-o-8HED

justinshreve added some commits Jun 16, 2015

@justinshreve

This comment has been minimized.

Show comment
Hide comment
@justinshreve

justinshreve Jun 16, 2015

Contributor

@dereksmart thanks for pointing those out - both should be fixed now. Passing our App ID and providing the fb-root div (its annoying a warning is presented when its not technically required..)

Contributor

justinshreve commented Jun 16, 2015

@dereksmart thanks for pointing those out - both should be fixed now. Passing our App ID and providing the fb-root div (its annoying a warning is presented when its not technically required..)

@dereksmart

This comment has been minimized.

Show comment
Hide comment
@dereksmart

dereksmart Jun 16, 2015

Member

wow that is annoying.

Anyway looks great now. Read, tested & works beautifully.

Member

dereksmart commented Jun 16, 2015

wow that is annoying.

Anyway looks great now. Read, tested & works beautifully.

@zinigor

This comment has been minimized.

Show comment
Hide comment
@zinigor

zinigor Jun 17, 2015

Member

Read the changes, tested, works as expected!

Member

zinigor commented Jun 17, 2015

Read the changes, tested, works as expected!

dereksmart added a commit that referenced this pull request Jun 17, 2015

Merge pull request #2237 from Automattic/update/facebook-likebox-widget
Replace the Facebook Like Box plugin with the Facebook Page Plugin (#1992)

@dereksmart dereksmart merged commit 8215c8e into master Jun 17, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dereksmart dereksmart deleted the update/facebook-likebox-widget branch Jun 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment