Widget Visibility: add ability to match ALL conditions. by zinigor · Pull Request #6327 · Automattic/jetpack

zinigor · 2017-02-08T11:56:03Z

This fixes issue #852. (Replay of #3289 by @atrus1701 )

Add the option to match all conditions specified for the widget.

This is related to Pull Request #3210. This original Pull Request contained several fixes that are being broken out into several smaller requests.

zinigor · 2017-02-08T11:56:57Z

Master issue: #5781

zinigor · 2017-02-09T12:51:04Z

I have added a couple of fixes:

The match_all flag would not work properly if any of the conditions were met, now the logic checks all conditions.
The checkbox is now under the conditions and has some padding.

georgestephanis

If we're going to add a "Match all" checkbox, wouldn't it make more sense to do it up top with the original prompt changed to two dropdowns -- something like ...

(Show|Hide) if (All|Any|None) of the following conditions match:

?

georgestephanis · 2017-02-15T21:53:03Z

modules/widget-visibility/widget-conditions.php

 							break;
 							case 'posts':
-								$condition_result = $wp_query->is_posts_page;
+								$condition_result = is_single();


This is a definite change -- is_posts_page returns true for the posts archive view when there's a page on the site's home. is_single() returns true on a page's permalink, non-archive view.

https://github.com/WordPress/WordPress/blob/9d7ea04936125041b224b227f48ad4194066b314/wp-includes/class-wp-query.php#L1008

georgestephanis · 2017-02-15T21:54:36Z

modules/widget-visibility/widget-conditions.php

-								<span class="condition-conjunction"><?php echo esc_html_x( 'or', 'Shown between widget visibility conditions.', 'jetpack' ); ?></span>
+								<span class="condition-conjunction">
+									<?php
+										echo esc_html_x( 'or', 'Shown between widget visibility conditions.', 'jetpack' );


This probably shouldn't be broken out onto it's own line from the php opening and closing tags.

georgestephanis · 2017-02-15T21:56:28Z

modules/widget-visibility/widget-conditions.php

 		if ( empty( $conditions['rules'] ) )
 			$conditions['rules'][] = array( 'major' => '', 'minor' => '', 'has_children' => '' );

+		if ( ! isset( $conditions['match_all'] ) )


Conditionals need braces.

Also, may be better to use empty() here.

georgestephanis · 2017-02-15T21:58:03Z

modules/widget-visibility/widget-conditions.php

 								} elseif ( $rule['minor'] == get_option( 'page_for_posts' ) ) {
 									// If $rule['minor'] is a page ID which is also the posts page
-									$condition_result = $wp_query->is_posts_page;
+									$condition_result = is_single();


Same concern as above. Breaking existing behavior by testing for something different.

georgestephanis · 2017-02-15T21:58:31Z

modules/widget-visibility/widget-conditions.php

+
+				// In case the match_all flag was set we quit on first failed condition
+				break;
+			} elseif(


Needs a space between elseif and (

georgestephanis · 2017-02-15T21:59:13Z

modules/widget-visibility/widget-conditions.php

+				break;
+			} elseif(
+				(
+					! isset( $instance['conditions']['match_all'] )


Again, may be more readable as empty() instead of negating.

georgestephanis · 2017-02-15T22:00:43Z

modules/widget-visibility/widget-conditions.php

+				'show' == $instance['conditions']['action']
+				&& ! $condition_result
+			)
+			|| (


I personally find broken out conditionals like this to be harder to read, but if we do, can we please do it as ) || ( instead?

zinigor · 2017-02-22T18:53:40Z

Thanks for the review George! I have fixed the problems you have found. As with the checkbox - I think we need a design review here. This is what the condition table currently looks like:

zinigor · 2017-02-22T18:59:40Z

For reference that's what it looks like in master. I see that the styles and images for some things are missing, I'll fix that. But the main thing is that we need to figure out where to put the checkbox best.

georgestephanis

I think it looks right now.

I'd still prefer a editorial review for clarity, so the checkbox makes it clear it's switching from ANY of the conditions to ALL of the conditions.

Perhaps we could add some JS to change the .condition-conjunction from or to and when the checkbox is selected?

It can probably merge as-is, if you open a subsequent issue for added clarity on that.

- Add the option to match all conditions specified for the widget. - see #852.

zinigor · 2017-02-27T11:04:36Z

I have rebased this PR and added the following changes:

JSHint no longer has any warnings about the JS file;
The delimiter between the conditions now displays "or"/"and" based on the status of the "Match all" checkbox:

Since @MichaelArestad has given this one a 👍 , I'm removing the Desing Needed label, and since @georgestephanis has given this a 👍 , I'm putting the PR into Ready to Merge mode.

dereksmart · 2017-02-28T14:41:14Z

modules/widget-visibility/widget-conditions.php

+			} elseif (
+				(
+					empty( $instance['conditions']['match_all'] )
+					|| $instance['conditions']['match_all'] !== '1'


should be yoda style

dereksmart · 2017-02-28T14:41:24Z

modules/widget-visibility/widget-conditions.php

-			if ( $condition_result )
+			if (
+				isset( $instance['conditions']['match_all'] )
+				&& $instance['conditions']['match_all'] == '1'


yoda style please

dereksmart

Upon checking out the branch, I'm seeing both or and for the conditionals.

Edit: FIXED after re-building

dereksmart

When there is only one conditional set, and the checkbox is not checked, I see and when I expect to see or or nothing at all:

Edit: FIXED after re-building

Fixed after re-building

dereksmart

Works as expected

@see

* Changelog: update stable tag and move changelog to changelog.txt Also remove old releases from readme.txt to keep the changelog tab short. * Changelog: add #5883 Also update the filter's docblock to match new version. * Changelog: add #5938 * Changelog: add #6298 * Changelog: add #3405 * Changelog: add #5941 * Changelog: add #6239 * Changelog: add #6281 * Changelog: add #6303 * Changelog: add #6018 * Changelog: add #6300 * Changelog: add #6296 * Changelog: add #6130 * Changelog: add #6292 * Readme: remove extra "on". * Changelog: add #6307 * Changelog: add #3297 * Changelog: add #6275 * Changelog: add #6321 * Changelog: add #6297 * Readme: update the support forum link anchor. Anchor changed when WordPress.org forums were updated to bbPress 2 * Readme: update list of a12s, it wasn't up to date anymore! * Changelog: add #6338 * Changelog: add #6337 * Changelog: add #6335 * Changelog: add #6333 * Testing List: first version of the 4.7 testing list. * Changelog: add #6332 * Changelog: add #6325 * Changelog: add #6326 * Changelog: add #6339 * Changelog: add #6342 * Changelog: add #6343 * Changelog: add #6346 * Changelog: add #6347 * Changelog: add #6279 * Changelog: add #6306 * Changelog: add #6312 * Changelog: add #6316 * Changelog: add #6171 * Changelog: add #6317 * Changelog: add #6246 * Changelog: add #6263 * Changelog: add #4220 * Changelog: add #5888 * Changelog: add #3406 * Changelog: add #3637 * Changelog: add #6320 * Changelog: add #5992 * Changelog: add #6322 * Changelog: add #6324 * Changelog: add #6352 * Changelog: add #6355 * Changelog: add #6360 * Changelog: add #6362 * Changelog: add #6369, #6382 * Changelog: add #6370 * Changelog: add #6375 * Changelog: add #6383 * Changelog: add #6384 * Changelog: add #6386 * Changelog: add #6395 * Changelog: add #6403 * Changelog: add #6406 * Changelog: add #6418 * Changelog: add #6419 * Changelog: add #6434 * Changelog: add #6446 * Changelog: add #6006 * Changelog: add #6096 * Changelog: add #6399 * Changelog: fix typo. @see #6331 (comment) * Changelog: add #6440 * Changelog: add #6443 * Changelog: add #6445 * Changelog: add #6463 * Changelog: add #6468 * Changelog: add #6471 * Changelog: add #6474 * Changelog: add #6480 * Changelog: add #6497 * Changelog: add #6499 * Changelog: add #6514 * Changelog: add #6267 * Changelog: add #5940 * Changelog: add #6492 * Changelog: add #5281 * Changelog: add #6327 * Changelog: add #6451 * Changelog: add #6525 * Changelog: add #6530

zinigor added [Feature] Widget Visibility [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Feb 8, 2017

matticbot added the Touches WP.com Files label Feb 8, 2017

zinigor mentioned this pull request Feb 8, 2017

Widget Visibility: add ability to match ALL conditions. #3289

Closed

zinigor requested a review from georgestephanis February 9, 2017 12:49

georgestephanis suggested changes Feb 15, 2017

View reviewed changes

jeherve added [Status] In Progress [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 Feb 16, 2017

georgestephanis approved these changes Feb 22, 2017

View reviewed changes

atrus1701 and others added 9 commits February 27, 2017 11:58

Widget Visibility: add ability to match ALL conditions.

bcf6aba

- Add the option to match all conditions specified for the widget. - see #852.

Fix the condition to properly match in case match_all was set.

d2e4123

Moved the match_all checkbox to the bottom.

854e365

Added braces and used empty instead of isset.

0d7fb82

Processed minor style issues.

439d675

Brought back old condition matches.

4e06c71

Fixed jshint errors.

ebeada5

Added a label wrapper over the text to make it clickable.

66ac5aa

Added varying control delimiters based on match all checkbox.

a71c15d

zinigor force-pushed the fix/visibility-condition-issues branch from eee6fdd to a71c15d Compare February 27, 2017 10:56

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Design Review Design has been added. Needs a review! [Status] Needs Review This PR is ready for review. labels Feb 27, 2017

dereksmart reviewed Feb 28, 2017

View reviewed changes

dereksmart requested changes Feb 28, 2017

View reviewed changes

dereksmart previously requested changes Feb 28, 2017

View reviewed changes

dereksmart approved these changes Feb 28, 2017

View reviewed changes

dereksmart merged commit 3bd8d9e into master Feb 28, 2017

dereksmart deleted the fix/visibility-condition-issues branch February 28, 2017 15:21

dereksmart added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 28, 2017

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Feb 28, 2017

jeherve added a commit that referenced this pull request Feb 28, 2017

Changelog: add #6327

be01ad6

This was referenced Mar 6, 2017

Widget Visibility: can't select any option #6574

Closed

Widget Visibility: add an AND condition #852

Closed

Conversation

zinigor commented Feb 8, 2017

Uh oh!

zinigor commented Feb 8, 2017

Uh oh!

zinigor commented Feb 9, 2017

Uh oh!

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

zinigor commented Feb 22, 2017

Uh oh!

zinigor commented Feb 22, 2017

Uh oh!

georgestephanis left a comment

Choose a reason for hiding this comment

Uh oh!

zinigor commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dereksmart left a comment • 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!

dereksmart left a comment • 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!

dereksmart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dereksmart left a comment •

edited

Loading

dereksmart left a comment •

edited

Loading