Skip to content

Add selective refresh support for Twitter, Facebook, Contact Info, Social Media Icon widgets (WP 4.5)#3543

Merged
zinigor merged 6 commits intoAutomattic:masterfrom
xwp:feature/selective-refresh-widget-support
Mar 23, 2016
Merged

Add selective refresh support for Twitter, Facebook, Contact Info, Social Media Icon widgets (WP 4.5)#3543
zinigor merged 6 commits intoAutomattic:masterfrom
xwp:feature/selective-refresh-widget-support

Conversation

@westonruter
Copy link
Copy Markdown
Contributor

This PR is start implementing support for the new Selective Refresh feature coming in WordPress 4.5.

  • De-duplicate Facebook and Twitter SDK inclusions between widget and shortcode integrations.
  • Enqueue assets in Customizer preview even if not used (yet) for the sake of selective refresh, in case a widget or embed is added without a full page refresh (which is what selective refresh is supposed to minimize).
  • Move locale methods from Facebook widget to Jetpack class.
  • Re-invoke Facebook, Twitter, and Google Maps JS libraries when re-rendering partials so that elements can be re-constructed.

For Customizer support for infinite scroll, see #3542. This PR includes the Jetpack widgets support from the Customize Partial Refresh plugin which was merged into Core for 4.5.

The customize_selective_refresh widget option is being introduced in https://github.com/xwp/wordpress-develop/pull/144 for #35855.

@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Mar 14, 2016
@jeherve jeherve added this to the 3.10 milestone Mar 14, 2016
@westonruter
Copy link
Copy Markdown
Contributor Author

Some more background on these changes: https://core.trac.wordpress.org/ticket/35855#comment:20

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Mar 16, 2016

Howdy @westonruter! Thanks for the PR. We've been chatting about the DRY commit specifically and the good work in this PR on that.

With regards to the selective refresh in the Customizer, is this PR backwards-compat with 4.3 and 4.4?

@westonruter
Copy link
Copy Markdown
Contributor Author

@kraftbj thanks!

With regards to the selective refresh in the Customizer, is this PR backwards-compat with 4.3 and 4.4?

Yes, it should be backwards-comaptible. This is what the hasSelectiveRefresh bit is for.

@zinigor zinigor self-assigned this Mar 17, 2016
@zinigor zinigor modified the milestones: 3.9.5, 3.10 Mar 18, 2016
@westonruter westonruter changed the title Add selective refresh support for Twitter, Facebook, and Contact Info widgets (WP 4.5) Add selective refresh support for Twitter, Facebook, Contact Info, Social Media Icon widgets (WP 4.5) Mar 19, 2016
@westonruter
Copy link
Copy Markdown
Contributor Author

I just amended the PR to include the customize_selective_refresh widget option is being introduced in https://github.com/xwp/wordpress-develop/pull/144 for #35855. I also added support for the Social Media Icons widget.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 22, 2016
@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Mar 22, 2016

Thank you! Tested adding, removing, moving around and modifying all related widgets, everything seems to work correctly!

@dereksmart
Copy link
Copy Markdown
Contributor

The code itself looks great and tests well.

We are going to have to figure out something in regards to how this is going to sync back to wpcom cleanly. But this will be a task for the Jetpack team.

@zinigor zinigor merged commit 58085e0 into Automattic:master Mar 23, 2016
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 23, 2016
$this->alt_option_name = 'widget_contact_info';

if ( is_customize_preview() ) {
add_action( 'wp_enqueue_scripts', array( $this, 'enqueue_scripts' ) );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @westonruter! is this really necessary? The scripts are enqueued when the map is built. Maybe I'm missing something because I think this might actually be disadvantageous if we do this for all widgets. We will end up with a lot of JS and CSS loaded in Customizer whether they're in use by a widget or not which will add to the Customizer loading times.
If possible, I'd love to talk for a bit in WordPress Slack whenever it's possible to you.
Cheers!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eliorivero the scripts are enqueued when the map is built… and the page is re-loaded. If adding a Contact Info widget for the first time to the page via selective refresh, those enqueues will be called but the scripts aren't going to be printed out because the footer is not re-rendered. So this is why enqueueing the JS up-front regardless will ensure the JS is available from the moment that a widget is added to the page.

It is true that this adds additional script bloat to the page, and there are two alternative solutions that could be done, although they require more work:

  1. Lazy-load the JS file when the widget is added for the first time if it is detected that the JS file is not already loaded. Actually you can see this in the original Jetpack support shim logic for the Customize Partial Refresh plugin: https://github.com/xwp/wp-customize-partial-refresh/blob/master/js/plugin-support/jetpack.js#L108-L137

  2. Keep track of the scripts that are enqueued and pass them back to the client in a selective refresh response, injecting the scripts into the page if they are not already present. You can see that the foundations for this are already present in Core via the customize_render_partials_response filter. Note the comment associated with that filter and how the scripts array item is noted as being reserved: https://github.com/xwp/wordpress-develop/blob/cfd07d69cfd87fdbc53b47b8aa156df140f64f0b/src/wp-includes/customize/class-wp-customize-selective-refresh.php#L405-L435 This would be taking the same approach as Jetpack's Infinite Scroll which also keeps track of the scripts enqueued and then outputs them to the page.

So, both of these two alternatives are ultimately better in the long run, although they require more work and thought to work out. In the short term, enqueueing the JS unconditionally if is_customize_preview() seemed like a good quick win to add the initial support, without having enough time to consider all of the edge cases before 4.5 is released.

Happy to chat about this in Slack as well. 🍻

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@westonruter thank you very much for the thorough explanation since it made me understand the functioning. The two examples are great and I'll definitely borrow some inspiration in the future to avoid loading everything at once. For now and given the short time before Apr 12, I agree that it makes sense to do it this way. I've started making the other widgets compatible following the approaches for the widgets in this branch.
Thanks again and have a nice weekend!

zinigor added a commit that referenced this pull request Feb 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants