Widgets: Porting of WP.com's EU Cookie Law Banner by Copons · Pull Request #6881 · Automattic/jetpack

Copons · 2017-04-03T19:17:53Z

This is my first PR in the effort of filling the gaps between Jetpack and WP.com, to smooth the AT process out.

I'm not completely sure about the Jetpack coding standards (file, classes and variables naming conventions, documentation) as apparently different widgets have different standards, so if you have any suggestions I'll be happy to implement them ASAP.

Changes proposed in this Pull Request:

Adds the EU Cookie Law Banner widget as already available on WP.com.

Testing instructions:

Add the widget to a sidebar and check if the banner shows up near the bottom of the window.
Check if, when closed with the selected method, it actually stays closed.
Check if both light and dark themes work appropriately.

zinigor

Overall this looks good, I see you have added textdomains for translations and namespaced functions. I have pointed out one minor thing, and have not yet tested the code. When you consider that the code is ready to test, feel free to remove the In Progress label and use the Needs Review label instead. One of the maintainers will test and review the code and will either assign a Ready to Merge label, or point out problems and put it back In Progress. In the former case the PR will be merged by another maintainer, in the latter you will need to make adjustments accordingly.
Thanks for doing this, we really appreciate the help! :)

zinigor · 2017-04-04T09:19:06Z

modules/widgets/eu-cookie-law.php

+
+		do_action( 'jetpack_stats_extra', 'widget_view', 'eu_cookie_law' );
+
+		echo 'CIAO';


I'm pretty sure that's not supposed to be here :)

That's totally required by EU laws!

Copons · 2017-04-04T09:51:30Z

@zinigor thanks for giving a look!
I've labeled In Progress mostly because I wanted more time to look at documentations standards in other JP widgets.
I could have reused this widget .com version docblocks, but there are internal links that I don't think are supposed to get into JP.

stoyan0v · 2017-04-04T10:32:42Z

modules/widgets/eu-cookie-law/footer.php

+		_e( 'To find out more, as well as how to remove or block these, see here:', 'jetpack' );
+		echo ' ';
+	} else {
+		echo esc_html( $instance['customtext'] ), ' ';


One thing that I notice is that you should put ,' ' in the brackets, and you should replace the empty space with jetpack textdomain.
I started this this widget while ago, but I hadn't enough time to complete it. Feel free to use any codeblock you like master...Stoyan0v:add/eu-cookie-law-widget

stoyan0v · 2017-04-04T10:46:04Z

modules/widgets/eu-cookie-law/footer.php

+		echo esc_html( $instance['customtext'] ), ' ';
+	} ?>
+
+	<a href="<?php echo esc_attr(


I think that using esc_url is better option here.
And also it will be nice if you move the logic outside of the esc_attr function.

jeherve

I left a few comments that should hopefully help a bit! Let me know if you have any questions.

jeherve · 2017-04-04T16:24:13Z

modules/widgets/eu-cookie-law.php

+	function __construct() {
+		parent::__construct(
+			'eu_cookie_law_widget',
+			apply_filters( 'jetpack_widget_name', esc_html__( 'EU Cookie Law Banner', 'jetpack' ) ),


All code in Jetpack is parsed after each release to build our codex:
http://developer.jetpack.com/

The codex references all actions and hooks in Jetpack, in order to help third-party developers who would like to customize Jetpack.

That means that you'll need to add docblocks for each one of the hooks you add in Jetpack. In this specific case, since the filter isn't new, this should do the trick:
/** This filter is documented in modules/widgets/facebook-likebox.php */

jeherve · 2017-04-04T16:25:57Z

modules/widgets/eu-cookie-law.php

+			'customtext'         => '',
+			'color-scheme'       => 'default',
+			'policy-url'         => 'default',
+			'default-policy-url' => 'https://en.support.wordpress.com/cookies',


Since Jetpack and WordPress.com use different cookies, you'll probably need to create a new support doc on jetpack.com for this widget.

jeherve · 2017-04-04T16:26:29Z

modules/widgets/eu-cookie-law.php

+			'custom-policy-url'  => '',
+			'policy-link-text'   => __( 'Our Cookie Policy', 'jetpack' ),
+			'button'             => __( 'Close and accept', 'jetpack' ),
+			'default-text'       => __( 'Privacy & Cookies: This site uses cookies from WordPress.com and selected partners. ', 'jetpack' ),


Since this site is self-hosted, it doesn't use cookies from WordPress.com (at least not for logged out readers).

All these should use the safest esc_html__

jeherve · 2017-04-04T16:27:16Z

modules/widgets/eu-cookie-law.php

+	public function widget( $args, $instance ) {
+		$this->instance = wp_parse_args( $instance, $this->defaults );
+
+		do_action( 'jetpack_stats_extra', 'widget_view', 'eu_cookie_law' );


/** This action is already documented in modules/widgets/gravatar-profile.php */

jeherve · 2017-04-04T16:28:44Z

modules/widgets/eu-cookie-law.php

+		require( dirname( __FILE__ ) . '/eu-cookie-law/footer.php' );
+	}
+
+	public function form( $instance ) {


It would be a nice plus to add docblocks before each one of your functions. If you install phpcs, you'll see that it complains when no comments are added:
https://github.com/Automattic/jetpack/blob/master/docs/coding-guidelines.md#general

I had to disable phpcs since it complained about the name of every class file. 😄

jeherve · 2017-04-04T16:34:25Z

modules/widgets/eu-cookie-law.php

+	}
+
+	function enqueue_style() {
+		wp_enqueue_style( 'eu-cookie-law-style', plugins_url( 'eu-cookie-law/style.css', __FILE__ ), array(), '20170403' );


Jetpack automatically concatenates all CSS files into a single jetpack.css file. To add your CSS to the concatenated file, you'll need to add it here and here.

jeherve · 2017-04-04T16:35:48Z

modules/widgets/eu-cookie-law/footer.php

+</div>
+
+<script type="text/javascript">
+	jQuery( function( $ ) {


We rely on jQuery here, but we didn't make sure it was actually enqueued on the page before.

I believe I should just enqueue this script and pass the values I need from PHP (cookie name and validity) in data attributes instead.

jeherve · 2017-04-04T16:38:02Z

modules/widgets/eu-cookie-law/form.php

+
+<p class="small">
+	<?php _e( 'It is your own responsibility to ensure that your site complies with the relevant laws.', 'jetpack' ); ?>
+	<a href="">


Should there be a link here?

Copons · 2017-04-04T17:04:54Z

@stoyan0v, @jeherve Thanks so much for the pointers and sorry for the missing stuff: I'm not used to work in PHP anymore and I had quite a long time of trial-and-errors debugging this!

eliorivero

Hi @Copons, good work spearheading this!

In addition to the comments:

the radio button to set a custom text can't be checked:

the button label in dark color scheme isn't visible

please introduce Selective Refresh support for this widgets like the other Jetpack widgets have

eliorivero · 2017-04-04T16:54:45Z

modules/widgets/eu-cookie-law.php

+			'custom-policy-url'  => '',
+			'policy-link-text'   => __( 'Our Cookie Policy', 'jetpack' ),
+			'button'             => __( 'Close and accept', 'jetpack' ),
+			'default-text'       => __( 'Privacy & Cookies: This site uses cookies from WordPress.com and selected partners. ', 'jetpack' ),


All these should use the safest esc_html__

eliorivero · 2017-04-04T16:56:19Z

modules/widgets/eu-cookie-law.php

+			'custom-policy-url'  => '',
+			'policy-link-text'   => __( 'Our Cookie Policy', 'jetpack' ),
+			'button'             => __( 'Close and accept', 'jetpack' ),
+			'default-text'       => __( 'Privacy & Cookies: This site uses cookies from WordPress.com and selected partners. ', 'jetpack' ),


There's a space at the end of the translatable string.

eliorivero · 2017-04-04T16:58:19Z

modules/widgets/eu-cookie-law/footer.php

+		?>
+		<br />
+		<?php
+		_e( 'To find out more, as well as how to remove or block these, see here:', 'jetpack' );


This and all similar cases could use the safest esc_html_e

eliorivero · 2017-04-04T17:02:36Z

modules/widgets/eu-cookie-law/footer.php

+			}
+
+			var expireTime = new Date();
+			expireTime.setTime( expireTime.getTime() + <?php echo $cookie_validity * 1000; ?> );


Needs esc_js

eliorivero · 2017-04-04T17:02:46Z

modules/widgets/eu-cookie-law/footer.php

+			var expireTime = new Date();
+			expireTime.setTime( expireTime.getTime() + <?php echo $cookie_validity * 1000; ?> );
+
+			document.cookie = '<?php echo $cookie_name; ?>=' + expireTime.getTime() + ';path=/;expires=' + expireTime.toGMTString();


Needs esc_js

eliorivero · 2017-04-04T17:04:46Z

modules/widgets/eu-cookie-law/form.php

@@ -0,0 +1,195 @@
+<p>
+	<strong>
+		<?php echo _x( 'Hide the banner', 'action', 'jetpack' ); ?>


Could use _ex

eliorivero · 2017-04-04T17:05:27Z

modules/widgets/eu-cookie-law/form.php

+	</ul>
+	<textarea
+		class="widefat"
+		name="<?php echo $this->get_field_name( 'text' ); ?>"


Needs esc_attr like it's done right below in the placeholder attribute.

eliorivero · 2017-04-04T17:06:00Z

modules/widgets/eu-cookie-law/form.php

+			<label>
+				<input
+					<?php checked( $instance['hide'], 'button' ); ?>
+					name="<?php echo $this->get_field_name( 'hide' ); ?>"


This and other similar cases need esc_attr.

eliorivero · 2017-04-04T17:23:29Z

One more:

Copons · 2017-04-04T17:26:31Z

Thanks @eliorivero!

I didn't expect such a thorough review on an in-progress PR.
You JP guys are so much nicer than those Calypso peeps! 😄

Copons · 2017-04-05T15:30:31Z

@eliorivero The various bugs on custom texts should be fixed but I cannot reproduce the dark color scheme button:

May I ask you what browser/OS you're using?

Copons · 2017-04-05T15:59:17Z

@jeherve I've discovered that there already is a Cookies policy page in JP, so I've simply replaced the old link with this one.

Related to this, I've removed the WP.com mention in the default banner text. Unfortunately, it becomes very short and ugly, but that's an easily fixable copy issue.
About copy, I've added a support page draft for this widget. I'll see to ping someone for the necessary checks.

eliorivero · 2017-04-07T20:19:17Z

@Copons looking great!

Sorry I forgot to mention this the first time: I got the white text on light grey button using TwentyTwelve theme. Chrome on El Capitan. Same thing happens in Firefox.

Copons · 2017-04-08T00:21:13Z

@eliorivero Thanks! Going to look into it.

Copons · 2017-04-10T12:13:50Z

@eliorivero It was a problem with Twenty Twelve styles that used background-image for the input background, that overrode this widget's background-color.

sirbrillig · 2017-04-20T21:22:09Z

This works for me!

However, I was unable to get the banner to show up in Twenty Seventeen if added to the "Blog Sidebar". It works if added to one of the "Footer" sidebars, though.

Any sidebar seems to work fine for me in Twenty Fifteen and Twenty Fourteen.

In Twenty Thirteen, it does not hover if not in the "Footer" sidebar:

Probably this behavior is expected since I get the same thing on WP.com sites, but I figured I'd mention it for the record since the testing instructions just read "add the widget to a sidebar", and not specifically a "footer" sidebar.

Copons · 2017-04-20T21:55:47Z

@sirbrillig At some point I got the same issue you're mentioning here, don't remember on what theme but likely on Twenty Seventeen, so this might be the solution for your problem.
Fact is, the homepage (set as a static page) has no sidebar, so no sidebar widgets will be rendered. If you try going in a blog page, you should see the banner.
It works in the footer because the homepage (every page actually) has the footer widget area. 😄

I gave it a quick look at Twenty Thirteen and I'd say that the problem there is that the footer area (called "Main Widget Area") is laid out via Masonry that adds an inline position: absolute that overrides the position: fixed from the widget stylesheet.
I think that all the solutions are just too hacky though: !important? Inline position: fixed? Ugh!

sirbrillig · 2017-04-20T23:40:26Z

oh! You're right about the home page sidebar! Good thinking.

As for Twenty Thirteen, yeah, I don't have any good ideas. In any case it's the same as dotcom.

kwight · 2017-04-21T19:54:51Z

@zinigor @jeherve @eliorivero anything else needed here? We're hoping to get it in for deadline next Tues, thanks!

jeherve · 2017-04-21T20:31:36Z

@kwight If you've reviewed and tested it, and are happy with it, do not hesitate to approve the PR. Someone else from Pit Crew will come along and test it again as well so it can be merged.

kwight

Checked form radio buttons, color scheme, overriding default texts, all looks good and ready to merge 👍

dereksmart

Other than the missing link, this looks good and tests well!

dereksmart · 2017-04-24T18:14:44Z

modules/widgets/eu-cookie-law/form.php

+
+<p class="small">
+	<?php esc_html_e( 'It is your own responsibility to ensure that your site complies with the relevant laws.', 'jetpack' ); ?>
+	<a href="">


Copons · 2017-04-24T21:12:13Z

@dereksmart Damn! I totally forgot about it.

On .com this is a link to a support page.
I've already created a draft on the JP support section (the most recent one), but I'd love a second look by some English speaker.
I'll add the permalink in advance though.

eliorivero · 2017-04-24T21:36:51Z

Works fine!

* 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

Copons added 2 commits April 3, 2017 18:58

Porting of the WP.com EU Cookie Law widget

77f70c1

Remove unnecessary comment left over from testing

1b44f3a

Copons added [Feature] Extra Sidebar Widgets [Status] In Progress Task labels Apr 3, 2017

zinigor suggested changes Apr 4, 2017

View reviewed changes

Remove a testing echo

4aa1bcd

stoyan0v reviewed Apr 4, 2017

View reviewed changes

jeherve requested changes Apr 4, 2017

View reviewed changes

eliorivero suggested changes Apr 4, 2017

View reviewed changes

Copons added 10 commits April 4, 2017 18:43

Add better escaping

7a8a4e1

Add hooks doc references

5685e79

Set style for concatenation

83e2e18

Enqueue script

a52022d

Cleanup and document the main widget file

d0acc40

Remove defaults from the front-end widget template

f78281e

Fix custom text from being un-saveable

a22e5c7

Enable selective refresh

5893f25

Move enqueued script to the footer

f09afbc

Fix selective refresh and widget positioning

22bf4ac

Use Jetpack Cookies policy URL and remove mentions of WP.com

4caed88

Copons added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Apr 5, 2017

Replace “wp_footer” mention with “bottom part of the screen”

6f467a8

Fix banner position and style on highly specific stylesheets

2ba51e4

jeherve added [Pri] High and removed [Pri] Normal labels Apr 21, 2017

kwight self-requested a review April 24, 2017 17:01

kwight approved these changes Apr 24, 2017

View reviewed changes

kwight 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

jeherve modified the milestone: 4.9 Apr 24, 2017

dereksmart requested changes Apr 24, 2017

View reviewed changes

zinigor approved these changes Apr 24, 2017

View reviewed changes

Copons added 2 commits April 24, 2017 22:13

Add missing support link

8678bce

Fix https instead of http

31b157a

eliorivero merged commit 16e8416 into master Apr 24, 2017

eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 24, 2017

eliorivero deleted the add/widgets/eu-cookie-law branch April 24, 2017 21:37

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

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

Changelog: add #6881

9ed29e9


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

		echo 'CIAO';

Conversation

Copons commented Apr 3, 2017

Changes proposed in this Pull Request:

Testing instructions:

Uh oh!

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

Copons commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

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!

Copons commented Apr 4, 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!

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!

eliorivero commented Apr 4, 2017

Uh oh!

Copons commented Apr 4, 2017

Uh oh!

Copons commented Apr 5, 2017

Uh oh!

Copons commented Apr 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

Copons commented Apr 8, 2017

Uh oh!

Copons commented Apr 10, 2017

stoyan0v Apr 4, 2017 •

edited

Loading

eliorivero commented Apr 7, 2017 •

edited

Loading