Add new Social Icons widget and deprecate old Social Media Icons widget by thomasguillot · Pull Request #7454 · Automattic/jetpack

thomasguillot · 2017-07-12T12:59:44Z

To keep in line with WPCOM, I’d like to add the new Social Icons
widget. You can add this widget to any site and you should have a new
widget as seen on its documentation page
(https://en.support.wordpress.com/widgets/social-media-icons-widget/).

The old widget should become unavailable to sites not using it. If they
do then we add a “(Deprecated)” to the label.

In social-icons.php, L72, we use the same social-menu.svg file as
the one used for the Social Menu.

georgestephanis · 2017-07-21T14:58:50Z

Working on a review now. CircleCI was removed -- do we have a good way to rerun it without?

We'll likely also need to git cherry-pick 963e3d5db9ef215c649a796062d5a2190f433f6c 5ab75dfce1718c60950580632db6a2b9b01b9547 in to account for the 5.2 and 5.3 breaking when travis re-runs.

georgestephanis · 2017-07-21T21:23:23Z

modules/widgets/social-icons.php

+	public function __construct() {
+		$widget_ops = array(
+			'classname'                   => 'jetpack_widget_social_icons',
+			'description'                 => esc_html__( 'Add social-media icons to your site.', 'jetpack' ),


Could we be escaping too early here? Would this string get escaped when actually used?

I'm concerned both with double escaping, and wrong-context escaping, if it's later used in an attribute and then gets re-escaped for that.

georgestephanis · 2017-07-21T21:23:27Z

modules/widgets/social-icons.php

+			'customize_selective_refresh' => true,
+		);
+
+		parent::__construct( 'jetpack_widget_social_icons', esc_html__( 'Social Icons', 'jetpack' ), $widget_ops );


Could we be escaping too early here? Would this string get escaped when actually used?

I'm concerned both with double escaping, and wrong-context escaping, if it's later used in an attribute and then gets re-escaped for that.

georgestephanis · 2017-07-21T21:23:40Z

modules/widgets/social-icons.php

+		parent::__construct( 'jetpack_widget_social_icons', esc_html__( 'Social Icons', 'jetpack' ), $widget_ops );
+
+		$this->defaults = array(
+			'title'     => esc_html__( 'Follow Us', 'jetpack' ),


Could we be escaping too early here?

georgestephanis · 2017-07-21T21:24:57Z

modules/widgets/social-icons.php

+		add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_admin_scripts' ) );
+		add_action( 'admin_print_footer_scripts', array( $this, 'render_admin_js' ) );
+		add_action( 'wp_enqueue_scripts', array( $this, 'enqueue_icon_scripts' ) );
+		add_action( 'wp_footer', array( $this, 'jetpack_social_menu_include_svg_icons' ), 9999 );


This looks bad.

Wouldn't this result in the SVGs printing on every wp_footer regardless of whether or not the widget is ever actually used, let alone used on that page (due to either multiple sidebars or widget visibility)?

georgestephanis · 2017-07-21T21:25:53Z

modules/widgets/social-icons.php

+	 * Script & styles for admin widget form.
+	 */
+	public function enqueue_admin_scripts() {
+		wp_enqueue_script( 'jetpack-widget-social-icons-script', plugins_url( 'social-icons/social-icons-admin.js', __FILE__ ), array( 'jquery-ui-sortable' ), '20170506' );


Wouldn't this enqueue these scripts for every admin page, including like ... post-new.php?

georgestephanis · 2017-07-21T21:38:45Z

modules/widgets/social-icons.php

+		$icon_count        = count( $new_instance['url-icons'] );
+		$instance['icons'] = array();
+
+		for ( $i = 0; $i < $icon_count; $i++ ) {


Why is this a for instead of a foreach?

georgestephanis · 2017-07-21T21:39:19Z

modules/widgets/social-icons.php

+	 * @return string|void
+	 */
+	public function form( $instance ) {
+		$instance = wp_parse_args( (array) $instance, $this->defaults );


Again, why cast to an array? wp_parse_args is designed to accept arg=val&arg2=val2 and the like. This would break that.

georgestephanis · 2017-07-21T21:39:43Z

modules/widgets/social-icons.php

+		$instance = wp_parse_args( (array) $instance, $this->defaults );
+		$title    = sanitize_text_field( $instance['title'] );
+		$sizes    = array(
+			'small'  => esc_html__( 'Small', 'jetpack' ),


I think we're escaping this too early, potentially double escaping.

georgestephanis · 2017-07-21T21:41:18Z

modules/widgets/social-icons.php

+		</p>
+
+		<?php
+			$support     = 'https://en.support.wordpress.com/widgets/social-media-icons-widget/#available-icons';


Should this maybe be a switch statement instead?

georgestephanis · 2017-07-21T21:42:10Z

modules/widgets/social-icons.php

+	<?php
+	}
+
+	/**


Note to self: This is as far as I got in my review, pick up from here later tonight.

georgestephanis · 2017-07-24T15:29:08Z

modules/widgets/social-icons.php

+
+				<p class="jetpack-widget-social-icons-remove-item">
+					<a class="jetpack-widget-social-icons-remove-item-button" href="javascript:;">
+						<?php esc_html_e( 'Remove', 'jetpack' ); ?>


This may need a translator note?

georgestephanis · 2017-07-24T15:30:22Z

modules/widgets/social-icons.php

+ * @access public
+ * @return void
+ */
+function jetpack_widget_social_icons_load() {


This could be a static method in the actual class, instead of being in the global namespace, but either way. Not really a blocker, just a thought.

georgestephanis · 2017-07-24T15:31:09Z

modules/widgets/social-icons.php

+}
+add_action( 'widgets_init', 'jetpack_widget_social_icons_load' );
+
+if ( ! function_exists( 'jetpack_social_menu_get_svg' ) ) :


Why is this in a function_exists? Do we have an alternate version wpcom side or the like?

georgestephanis · 2017-07-24T15:33:54Z

modules/widgets/social-icons.php

+function jetpack_social_menu_get_svg( $args = array() ) {
+	// Make sure $args are an array.
+	if ( empty( $args ) ) {
+		return esc_html__( 'Please define default parameters in the form of an array.', 'jetpack' );


Should this maybe be an error_log instead of printing on the front end? Unsure.

georgestephanis · 2017-07-24T15:34:33Z

modules/widgets/social-icons.php

+	$svg = '<svg class="icon icon-' . esc_attr( $args['icon'] ) . '"' . $aria_hidden . ' role="img">';
+
+	/*
+	 * Display the icon.


Note: So this just prints a reference to the icon that is elsewhere in the DOM. Doesn't read the file in or anything.

georgestephanis · 2017-07-24T15:42:39Z

modules/widgets/social-icons/social-icons-admin.css

+	font: 400 20px/1 dashicons;
+	vertical-align: middle;
+	-webkit-transition: all .2s;
+	transition: all .2s;


Missing leading zero on the .2s

georgestephanis · 2017-07-24T15:44:19Z

modules/widgets/social-icons/social-icons-admin.js

+
+	// Live preview update.
+	function livePreviewUpdate( button ) {
+		if ( ! $( 'body' ).hasClass( 'wp-customizer' ) || ! button.length  ) {


May be more efficient to use document.body or -- if jQuery is needed -- $( document.body ) instead of passing in a string element selector? Unsure if jQuery has some optimization behind the scenes for this already.

georgestephanis · 2017-07-24T15:46:25Z

modules/widgets/social-icons/social-icons-admin.js

+		} );
+
+		// Event handler for widget open button.
+		$( document ).on( 'click', 'div.widget[id*="jetpack_widget_social_icons"] .widget-title, div.widget[id*="jetpack_widget_social_icons"] .widget-action', function() {


Yay for consistently using double quotes around attribute selector values! 👍

georgestephanis · 2017-07-24T15:47:55Z

modules/widgets/social-icons/social-icons.css

+
+.jetpack_widget_social_icons svg {
+	color: inherit;
+	fill: currentColor;


Nice!

https://caniuse.com/#feat=currentcolor

georgestephanis · 2017-07-24T15:48:59Z

modules/widgets/social-media-icons.php

 */
 function wpcom_social_media_icons_widget_load_widget() {
+	// [DEPRECATION]: Only register widget if active widget exists already
+	$has_widget = is_active_widget( false, false, 'wpcom_social_media_icons_widget', false );


Are we sure this works? Testing if a widget is active before registering it?

Also, it seems like this may be more efficient to cache in a transient rather than having WordPress scan through every sidebar on every page load. That's probably a blocker.

georgestephanis · 2017-07-24T15:49:59Z

^^ there's the end of the review. Please ping me in Slack if you have any questions, I'd be delighted to work with anyone and answer any questions about my remarks. <3

mendezcode · 2017-07-24T15:55:21Z

Howdy @georgestephanis, I'll be having a look at this and will tackle the fixes accordingly. I'll keep you posted on Slack if I have any questions. Thanks!

jeherve · 2017-09-07T13:54:32Z

This would fix #7433

jeherve

A few changes to keep the new widget more in line with the old one.

jeherve · 2017-10-05T10:01:03Z

modules/widgets/social-icons.php

+			'customize_selective_refresh' => true,
+		);
+
+		parent::__construct( 'jetpack_widget_social_icons', __( 'Social Icons', 'jetpack' ), $widget_ops );


Could you add the jetpack_widget_name filter here, like so?

/** This filter is documented in modules/widgets/facebook-likebox.php */ apply_filters( 'jetpack_widget_name', __( 'Social Icons', 'jetpack' ) ),

jeherve · 2017-10-05T10:09:52Z

modules/widgets/social-icons.php

+			return;
+		}
+
+		// Define SVG sprite file in Jetpack -- NEEDS TESTING


I must admit I'm not super clear on the differences between the svg we have in our Theme Tools and social logos, that we use for Jetpack Sharing buttons:
https://github.com/Automattic/jetpack/tree/master/_inc/social-logos

Is that SVG sprite the same as the one available here?
https://github.com/Automattic/social-logos/tree/master/svg-sprite

Could we use the icon font instead, since it may be loaded by the sharing buttons on most pages where the widget (or the social menu) will also be displayed? It would be better for performance, and we would only have to update the icon font when the social logos get updated.

What do you think?

This is the same SVG file used for the Social Menus: https://github.com/Automattic/jetpack/blob/master/modules/theme-tools/social-menu/social-menu.svg

It's much more flexible than an icon font and it supports more social networks.

jeherve · 2017-10-05T10:13:40Z

modules/widgets/social-icons.php

+
+					<?php if ( ! empty( $icon['url'] ) ) : ?>
+						<li class="jetpack-social-widget-item">
+							<a href="<?php echo esc_url( $icon['url'], array( 'http', 'https', 'mailto', 'skype' ) ); ?>"<?php echo $new_tab; ?>>


We used to have a jetpack_social_media_icons_widget_profile_link filter in the old widget. Maybe we should add it here too?
https://developer.jetpack.com/hooks/jetpack_social_media_icons_widget_profile_link/

jeherve · 2017-10-05T10:15:08Z

modules/widgets/social-icons.php

+			),
+		);
+
+		return $social_links_icons;


Should we allow folks to customize that list so they can add their own icon, as we used to allow with jetpack_social_media_icons_widget_array?
https://developer.jetpack.com/hooks/jetpack_social_media_icons_widget_array/

We already support much more social networks than the old widget. Also, for consistency reason, I would not recommend adding a filter here.

Same with jetpack_social_media_icons_widget_profile_link and jetpack_social_media_icons_widget_output

jeherve · 2017-10-05T10:16:20Z

modules/widgets/social-icons.php

+		<?php
+		endif;
+
+		echo $args['after_widget'];


Could you add the jetpack_stats_extra action here, like we do for other widgets?

jetpack/modules/widgets/social-media-icons.php

Line 217 in 1b19c46

do_action( 'jetpack_stats_extra', 'widget_view', 'social_media_icons' );

jeherve · 2017-10-05T10:17:26Z

modules/widgets/social-icons.php

+		<?php
+		endif;
+
+		echo $args['after_widget'];


We also add a jetpack_social_media_icons_widget_output filter here so folks could customize the whole output. Maybe we should do the same in that new widget?
https://developer.jetpack.com/hooks/jetpack_social_media_icons_widget_output/

* Add V1.2 Plugin Endpoints * simply eveything * Updates as requested.. These change need to be tested better. * Stash things? * Add log back in. Refactor to use the _response_format_v1_2 format array. * Remove the .com only api endpoints. From jetpack Remove * Add log to plugin format method * Minor fixes * minor code fixes

* Shortcodes: remove aws domain from Mailchimp shortcode. Fixes #7945 To test, create a Maichimp form as explained here: https://en.support.wordpress.com/mailchimp/ Then try to embed it into one of your posts or pages, when logged in as an editor. You should see the form transform into a shortcode on save. Make sure the form is displayed properly on the site. * Mailchimp: save a shortcode when pasting a form code in the widget Fixes #7960 Fixes #7990 - The widget now calls the shortcode file even when the shortcodes are not active, so you do not need to activate the shortcode module for this to work. - On save, the widget uses the reversal function provided in the shortcode to save a valid shortcode. - That shortcode is then displayed in the widget, as before. * Mailchimp shortcode: update failing unit test.

CLI: Do not change directory within partner scripts

Recently we added some bug fixes. This introduced a bug where we would always be returning an error when we install an plugin. This PR fixes this by making sure that $error is being checked for the empty case.

Plugin API: Installing plugins always returns an error.

This parameter has been present since 4.2 and by applying the filter here, we are currently breaking all callbacks that expect the $instance

* Sync: Fix issue with syncing raw URLs for multisites * Sync: Fix failing test after updating multisite logic

#8381)

This ensures that nested blocks are replaced first (e.g. code inside shortcodes).

…#8245)

…#8392) * Hide all mentions of VaultPress is rewind is available but not active * removed commented code

* First pass -- VaultPress deactivation if Rewind is running. I may (probably will) need to revisit where in the `$features` array I'm checking to see if Rewind is enabled. * Add the file to the included list for 3rd-party * Retool to new method to check for Rewind availability. * Update is_rewind_enabled() method to read the correct data for determining status of Rewind * VaultPress Murder * faulty logic

* Dashboard: fix issue where Jetpack version at the footer was shown as undefined since data wasn't available on the static page. This will now show `Jetpack` when the version is unavailable and the complete Jetpack version xx when it's available.

To keep in line with WPCOM, I’d like to add the new Social Icons widget. You can add this widget to any site and you should have a new widget as seen on its documentation page (https://en.support.wordpress.com/widgets/social-media-icons-widget/). The old widget should become unavailable to sites not using it. If they do then we add a “(Deprecated)” to the label. In `social-icons.php`, L72, we use the same `social-menu.svg` file as the one used for the Social Menu.

* Fix double escaping * Remove browser prefix from CSS * Use `document.body` in admin JS * Move Social Icons functions (SVG, icons and label) inside the widget class * Only load admin scripts when needed (widget.php and customizer) * Only load SVG footer file when widget is active * Use `is_file()` instead of `file_exists()` to check if the SVG icons file exists * Use `stripos()` instead of `strpos` to parse the URL * Use a `switch` statement instead of `if` to detect `get_locale` * Set the default icon (chain) outside the `foreach` loop * Refactor `get_supported_icons()` and only use one function for icon name and label instead of 2 * Fix `esc_url` for Skype links `skype:` * Add option to open links in a new tab * Add transient to old widget to check if it's active or not

…ck_stats_extra`

… using `empty()` with arbitrary expressions

…etpack into social-icons-widget

jeherve · 2017-12-29T17:34:21Z

Looks like something went wrong with that rebase @thomasguillot. Could you give it another try?

…et. Closes #7454 Closing previous PR/branch and opening a new one due to an issue with a rebase.

thomasguillot · 2018-01-10T13:18:41Z

I've opened a new PR, #8498, as I'm not quite sure what happened here and this is a mess.

…et (#8498) * Add new Social Icons widget and deprecate old Social Media Icons widget. Closes #7454 Closing previous PR/branch and opening a new one due to an issue with a rebase. * Social Media Icons: Remove the strict type check to only register widget if active widget exists already * Social Icons widget: Set target attribute for the link Will set the target attribute to `_blank` or `_self` depending on the new tab option being ticked or not.

matticbot added the Touches WP.com Files label Jul 12, 2017

thomasguillot added [Status] Needs Review This PR is ready for review. [Type] Dotcom Merge labels Jul 14, 2017

mendezcode self-assigned this Jul 17, 2017

georgestephanis suggested changes Jul 21, 2017

View reviewed changes

georgestephanis suggested changes Jul 24, 2017

View reviewed changes

jeherve added the [Feature] Extra Sidebar Widgets label Sep 7, 2017

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Sep 7, 2017

This was referenced Oct 2, 2017

Social Media Icons Widget: add new parameters #7917

Closed

Social Icons Widget #7915

Closed

thomasguillot requested a review from a team as a code owner October 3, 2017 09:11

jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Oct 3, 2017

jeherve requested changes Oct 5, 2017

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Oct 5, 2017

enejb and others added 9 commits October 18, 2017 15:27

Merge pull request #8009 from Automattic/fix/partner-provision-cd

441289e

CLI: Do not change directory within partner scripts

Plugin API: Installing plugins always returns an error.

8730b69

Recently we added some bug fixes. This introduced a bug where we would always be returning an error when we install an plugin. This PR fixes this by making sure that $error is being checked for the empty case.

Merge pull request #8017 from Automattic/fix/jetpack-install-plugin-bug

9207e60

Plugin API: Installing plugins always returns an error.

Remove the need for sending user roles (#8013)

603f598

do post_gallery action with integer as third parameter. (#8015)

e81a75f

This parameter has been present since 4.2 and by applying the filter here, we are currently breaking all callbacks that expect the $instance

Sync: Fix issue with syncing raw URLs for multisites (#8006)

ca9c9c9

* Sync: Fix issue with syncing raw URLs for multisites * Sync: Fix failing test after updating multisite logic

CLI: Update script so that global args are directly after wp

f5280b1

eliorivero and others added 14 commits December 20, 2017 10:43

Docs: remove VP references (#8373)

fb4c3f5

Search: Fixes an undefined index when filters were disabled via filter (

9f2d474

#8381)

theme-option: Use esc_html__() in customize_register function. (#8360)

27a9e61

WP CLI: remove VP reference (#8375)

33d2309

Reverse text hashes before restoration (#7536)

9dd2cc1

This ensures that nested blocks are replaced first (e.g. code inside shortcodes).

JITM: Correct API response shape when the 0 index has been dismissed. (…

11164da

…#8245)

Comments: sync comment endpoint with dotcom (#8346)

23debd3

Update Goodreads URL for HTTPS support (#8364)

92004a8

Hide all mentions of VaultPress if rewind is available but not active (…

0cc673c

…#8392) * Hide all mentions of VaultPress is rewind is available but not active * removed commented code

Related Posts: add docblock to existing filter (#8359)

c4978c2

Add revisions to Portfolios (#8307)

a6db31f

Add yarn clean-client before building the Admin Page (#8365)

e1ebfd8

oskosk mentioned this pull request Dec 21, 2017

Social Media Icons Widget: add new parameters #8397

Closed

oskosk and others added 7 commits December 21, 2017 11:20

Admin Page: Hide Masterbar card for Atomic Sites (#8290)

d0c8ef3

Social Icons Widget: Add filters for jetpack_widget_name and `jetpa…

0a9d8f8

…ck_stats_extra`

Social Icons Widget: Add missing semicolon

683e044

Social Icons Widget: Create new variable $is_active_widget to avoid…

d8b6b1c

… using `empty()` with arbitrary expressions

Merge branch 'social-icons-widget' of https://github.com/Automattic/j…

9e6fdb0

…etpack into social-icons-widget

thomasguillot requested a review from kraftbj as a code owner December 21, 2017 17:16

thomasguillot added a commit that referenced this pull request Jan 10, 2018

Add new Social Icons widget and deprecate old Social Media Icons widg…

4043dcf

…et. Closes #7454 Closing previous PR/branch and opening a new one due to an issue with a rebase.

thomasguillot mentioned this pull request Jan 10, 2018

Add new Social Icons widget and deprecate old Social Media Icons widget #8498

Merged

thomasguillot closed this Jan 10, 2018

thomasguillot deleted the social-icons-widget branch January 10, 2018 13:19

+              	<?php
+              	}
+              	/**

Conversation

thomasguillot commented Jul 12, 2017 • edited by jeherve Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgestephanis commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Jul 24, 2017

Uh oh!

mendezcode commented Jul 24, 2017

Uh oh!

jeherve commented Sep 7, 2017

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeherve commented Dec 29, 2017

Uh oh!

thomasguillot commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

thomasguillot commented Jul 12, 2017 •

edited by jeherve

Loading