Widgets: add missing internet-defense-league widget by gwwar · Pull Request #7030 · Automattic/jetpack

gwwar · 2017-04-21T20:11:13Z

This is part of 185-gh-customization

Changes proposed in this Pull Request:

This adds a wpcom widget to Jetpack. This is copied mostly verbatim from wpcom, with some small modifications for stats and the registration of the module.

Testing instructions:

In your test sandbox, navigate to your Jetpack plugin and checkout this branch
Navigate to the customizer /wp-admin/customize.php
Click on widgets
Then select a widget area
Click add a widget
With a connected Jetpack site, you should be able to search for Internet Defense League
Add it to your sidebar
The widget options should work, (different img types/visibility)
Publish your page
We should see that the Jetpack site now has an option of widget_internet_defense_league_widget which might have a shape like

jeherve

I left a few comments of things that could be fixed.

jeherve · 2017-04-21T22:04:54Z

modules/widgets/internet-defense-league.php

+	function __construct() {
+		parent::__construct(
+			'internet_defense_league_widget',
+			apply_filters( 'jetpack_widget_name_widget', esc_html__( 'Internet Defense League', 'jetpack' ) ),


jetpack_widget_name is the right filter name here, and you'll need to add a docblock in front of that filter. Something like /** This filter is documented in modules/widgets/facebook-likebox.php */

I'm curious, why not link to a doc page or simply copy the doc here?

/** * Filter the name of a widget included in the Extra Sidebar Widgets module. * * @module widgets * * @since 2.1.2 * * @param string $widget_name Widget title. */

Updated in c960d98

@gwwar We only include the full docblock once, so the parser doesn't create multiple entries for the same hook. That's how WordPress Core works as well.

jeherve · 2017-04-21T22:05:32Z

modules/widgets/internet-defense-league.php

+			'internet_defense_league_widget',
+			apply_filters( 'jetpack_widget_name_widget', esc_html__( 'Internet Defense League', 'jetpack' ) ),
+			array(
+				'description' => __( 'Show your support for the Internet Defense League.', 'internetdefenseleague' ),


You'll need to use the jetpack textdomain everywhere in that file, so strings can be picked up and translated.

Does this look about right in d340db8 ?

jeherve · 2017-04-21T22:05:56Z

modules/widgets/internet-defense-league.php

+		// When enabling campaigns other than 'none' or empty, change $no_current to false above.
+		$this->campaigns = array(
+			''       => __( 'All current and future campaigns', 'internetdefenseleague' ),
+			// 'nsa' => __( 'NSA Protest on July 4th, 2013', 'internetdefenseleague' ),


Since this doesn't apply anymore, you can remove it I think.

Oh good catch, it looks like we can hide the Don't display a badge (just the campaign) option as well.

Updated the badge options in 84411a5

jeherve · 2017-04-21T22:07:20Z

modules/widgets/internet-defense-league.php

+			array(
+				'description' => __( 'Show your support for the Internet Defense League.', 'internetdefenseleague' ),
+			)
+		);


I would suggest enabling Customizer's Selective Refresh, as mentioned here:
https://github.com/Automattic/jetpack/blob/master/docs/coding-guidelines.md#widgets

The widget is very simple, so it appears that adding the flag is enough in 7b37c44. Let me know if I missed anything though.

jeherve · 2017-04-21T22:08:07Z

modules/widgets/internet-defense-league.php

+			echo $args['before_widget'];
+			echo '<p><a href="https://internetdefenseleague.org/"><img src="' . esc_url( 'https://internetdefenseleague.org/images/badges/final/' . $instance['badge'] . '.png' ) . '" alt="Member of The Internet Defense League" style="max-width: 100%; height: auto;" /></a></p>';
+			echo $args['after_widget'];
+			do_action( 'jetpack_stats_extra', 'widget_view', 'internet_defense_league' );


Could you add a docblock before the action? /** This action is already documented in modules/widgets/gravatar-profile.php */ should do.

Added in f357d56

jeherve · 2017-04-21T22:09:29Z

modules/widgets/internet-defense-league.php

+				$instance['badge'] = $this->defaults['badge'];
+			}
+			echo $args['before_widget'];
+			echo '<p><a href="https://internetdefenseleague.org/"><img src="' . esc_url( 'https://internetdefenseleague.org/images/badges/final/' . $instance['badge'] . '.png' ) . '" alt="Member of The Internet Defense League" style="max-width: 100%; height: auto;" /></a></p>';


How about serving that image with Photon, thanks to jetpack_photon_url()?

Updated in 70ca32a

Should I also add translation strings here for the alt text?

Yes, it would be nice!

jeherve · 2017-04-21T22:11:01Z

modules/widgets/internet-defense-league.php

+			$this->campaign = $instance['campaign'];
+			$this->variant  = $instance['variant'];
+			add_action( 'wp_footer', array( $this, 'footer_script' ) );
+			do_action( 'jetpack_stats_extra', 'widget_view', 'internet_defense_league' );


Instead of adding the same action twice, how about placing it at the end of the widget output, so it's always triggered when the widget is in use, and always triggered only once?

I think this should be safe now with 84411a5. Previously I think it was possible for you to select none for a badge, and with no active campaigns this would bump stats even if nothing was rendered.

Updated in f357d56

jeherve · 2017-04-21T22:13:48Z

modules/widgets/internet-defense-league.php

+	}
+
+	public function footer_script() {
+		if ( ! isset( $this->campaigns[ $this->campaign ] ) )


You'll need to add opening and closing brackets here, as per the WP coding standards:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style

Updated in 1d9b30a

jeherve · 2017-04-21T22:14:00Z

modules/widgets/internet-defense-league.php

+		if ( ! isset( $this->campaigns[ $this->campaign ] ) )
+			$this->campaign = $this->defaults['campaign'];
+
+		if ( ! isset( $this->variants[ $this->variant ] ) )


Missing brackets here as well.

Updated in 1d9b30a

jeherve · 2017-04-21T22:15:32Z

modules/widgets/internet-defense-league.php

+
+**************************************************************************/
+
+class Internet_Defense_League_Widget extends WP_Widget {


Maybe we could prefix it with Jetpack_ to avoid any potential conflicts with other plugins or themes that may use the same class name?

👍 Updated in 76a219a

gwwar · 2017-04-21T23:31:20Z

Thanks for the detailed review @jeherve 👍 This is ready for another look when folks have time.

dereksmart · 2017-04-24T19:31:43Z

modules/widgets/internet-defense-league.php

+		parent::__construct(
+			'internet_defense_league_widget',
+			/** This filter is documented in modules/widgets/facebook-likebox.php */
+			apply_filters( 'jetpack_widget_name_widget', esc_html__( 'Internet Defense League', 'jetpack' ) ),


the correct filter name is jetpack_widget_name

Good catch there 👍

dereksmart · 2017-04-24T19:34:29Z

modules/widgets/internet-defense-league.php

+		);
+
+		if ( $this->no_current === false ) {
+			$this->badges[ 'none' ] = __( 'Don\'t display a badge (just the campaign)', 'jetpack' );


Coding standards say whitespace only for variables in the bracket

dereksmart · 2017-04-24T19:38:37Z

modules/widgets/internet-defense-league.php

+
+**************************************************************************
+
+Plugin Name:  Internet Defense League


I think we can remove this doc stuff. The format is kinda weird and it's not included in other widgets. Also not a plugin :)

dereksmart · 2017-04-24T19:39:00Z

Looks great aside from my comments above!

gwwar · 2017-04-24T19:53:03Z

Thanks for the reviews @dereksmart @jeherve! Let me know if I need to tidy anything else

eliorivero

Thanks for working on this @gwwar! There are some changes that would be great to make before merging.

eliorivero · 2017-04-24T22:03:19Z

modules/widgets/internet-defense-league.php

+}
+
+function jetpack_internet_defense_league_init() {
+	if ( Jetpack::is_active() ) {


This widget doesn't require anything from wpcom so it doesn't need a connection. This condition can be removed.

eliorivero · 2017-04-24T22:04:05Z

modules/widgets/internet-defense-league.php

+		// Hide first two form fields if no current campaigns.
+		if ( false === $this->no_current ) {
+			echo '<p><label>';
+			echo __( 'Which Internet Defense League campaign do you want to participate in?', 'jetpack' ) . '<br />';


This and similar instances should use esc_html_e() (or _e() when necessary) instead of echo __()

eliorivero · 2017-04-24T22:04:55Z

modules/widgets/internet-defense-league.php

+			$badge_url = esc_url( 'https://internetdefenseleague.org/images/badges/final/' . $instance['badge'] . '.png' );
+			$photon_badge_url = jetpack_photon_url( $badge_url );
+			echo $args['before_widget'];
+			echo '<p><a href="https://internetdefenseleague.org/"><img src="' . $photon_badge_url . '" alt="Member of The Internet Defense League" style="max-width: 100%; height: auto;" /></a></p>';


The string Member of The Internet Defense League must be translatable.

eliorivero · 2017-04-24T22:12:01Z

modules/widgets/internet-defense-league.php

+			)
+		);
+
+		// When enabling campaigns other than 'none' or empty, change $no_current to false above.


All the code associated to $no_current being false is never used since there's no way for user to change the property, which is always true, so maybe it should be removed.

I assume this was dev note in case another campaign was to be added, so someone could modify this quickly. If we remove this, we should remove the campaigns array as well since there are no active campaigns to choose from.

eliorivero · 2017-04-24T22:15:01Z

modules/widgets/internet-defense-league.php

+			/** This filter is documented in modules/widgets/facebook-likebox.php */
+			apply_filters( 'jetpack_widget_name', esc_html__( 'Internet Defense League', 'jetpack' ) ),
+			array(
+				'description' => __( 'Show your support for the Internet Defense League.', 'jetpack' ),


We usually use the safer esc_html__() instead of __().

gwwar · 2017-04-24T22:43:33Z

@eliorivero updated in f3d2612

eliorivero · 2017-04-24T23:43:11Z

Fantastic! Merging now.

* Changelog: initial commit for 4.9 release. * Changelog: add #6929 * Changelog: move old changelogs to changelog.txt * Readme: restore deleted release post link. The post is now live. * Changelog: add #6853 * Changelog: add #6856 * Changelog: add #6857 * Changelog: add #6884 * Changelog: add #6885 * Changelog: add #6892 * Changelog: add #6894 * Changelog: add #6898 * Changelog: add #6899 * Changelog: add #6900 * Changelog: add #6909 * Changelog: add #6927 * Changelog: add #6947 * Chagelog: add #6958 * Changelog: add #6961 * Changelog: add #6963 * Changelog: add #6965 * Changelog: add #6986 * Changelog: add #7000 * Changelog: add #7013 * Changelog: add #7015 * Changelog: add #7019 * Changelog: add #7028 * Changelog: add #6998 * Changelog: add #6999 * Changelog: add #7044 * Changelog: add #6881 * Changelog: add #6922 * Changelog: add #6940 * Changelog: add #6962 * Changelog: add #6942 * Changelog: add #6959 * Changelog: add #7018 * Changelog: add #6948 * Changelog: add #6657 * Changelog: add #7030 * Changelog: add #7048 * Changelog: add #7031 * Changelog: add #6990 * Changelog: add #6957 * Changelog: add #7027

Widgets: add missing internet-defense-league widget

031309d

gwwar added the [Status] Needs Review This PR is ready for review. label Apr 21, 2017

gwwar self-assigned this Apr 21, 2017

gwwar requested review from eliorivero, jeherve and zinigor April 21, 2017 21:57

jeherve added [Feature] Extra Sidebar Widgets [Pri] Normal New Feature labels Apr 21, 2017

jeherve requested changes Apr 21, 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 Apr 21, 2017

gwwar added 8 commits April 21, 2017 22:27

Widget: prefix class name with jetpack

76a219a

Widget: point to filter documentation

c960d98

Widget: update widget to use jetpack textdomain

d340db8

Widget: do not show no badge option when there are no active campaigns

84411a5

Widget: use photon

70ca32a

Widget: simplify stats bump and add doc block

f357d56

Widget: fix brace style

1d9b30a

Widget: use customize_selective_refresh

7b37c44

gwwar 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 Apr 21, 2017

jeherve modified the milestone: 4.9 Apr 24, 2017

dereksmart requested changes Apr 24, 2017

View reviewed changes

Widget: fix filter name, remove old doc, fix whitespace

9294994

dereksmart 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 Apr 24, 2017

dereksmart approved these changes Apr 24, 2017

View reviewed changes

eliorivero suggested changes Apr 24, 2017

View reviewed changes

eliorivero added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 24, 2017

Widget: update old usages of __, remove Jetpack connection check

f3d2612

gwwar removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Apr 24, 2017

Widget: update missed usage of __

1f05da2

eliorivero approved these changes Apr 24, 2017

View reviewed changes

eliorivero merged commit 00ff093 into master Apr 24, 2017

eliorivero deleted the add/internet-defense-league-widget branch April 24, 2017 23:43

eliorivero removed the [Status] Needs Review This PR is ready for review. label Apr 24, 2017

jeherve added a commit that referenced this pull request Apr 24, 2017

Changelog: add #7030

5898f1d

jeherve added the [Status] Has Changelog label Apr 24, 2017


		**************************************************************************/

		class Internet_Defense_League_Widget extends WP_Widget {


		**************************************************************************

		Plugin Name: Internet Defense League

Conversation

gwwar commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

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!

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!

gwwar commented Apr 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!

dereksmart commented Apr 24, 2017

Uh oh!

gwwar commented Apr 24, 2017

Uh oh!

eliorivero 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!

gwwar commented Apr 21, 2017 •

edited

Loading

gwwar Apr 24, 2017 •

edited

Loading