Implemented query panel in latest posts block. #3198

jorgefilipecosta · 2017-10-27T11:41:13Z

Description

Ths PR aims to implement the query panel specified in issue #2662.

The panel is added to latest posts block. But this leaves a discussion point, it allows, for example, to show 8 oldest posts by setting sorting from oldest to newest.
To solve this we need to make a decision :
We rename this block to "Posts" allowing to show the most recent ones or oldest or other option the user chooses.
Or we maintain the most recent posts concept, we always retrieve from the database the N most recent posts and then we sort this recent posts in accordance with the sorting criteria the user selected.
Feel free to share your options.

How Has This Been Tested?

Try to change the values of the 3 different options in the query panel. Verify that the posts in the editor update in accordance with the option selected. Save the post on each option change, and open the post, verify the server side render also shows the selected option correctly.

Screenshots (jpeg or gifs if applicable):

Notes

There is a bug where component looks like loading forever if there are no posts that become more visible with this changes (because we can more easily select options without posts). A fix is being issued in #3180.

The function buildTermsTree in utils was extracted from HierarchicalTermSelector component. As a future PR, this component will be refactored to make use of the utils function (removing the duplicate version) and will also be simplified/refactored to make use of the new component TermTreeSelect created in this PR.

aduth

Personally I'd have preferred to see the Latest Posts block updated to use withAPIData before enhancing it to handle additional data attributes.

aduth · 2017-10-30T12:52:51Z

blocks/library/latest-posts/index.php

+			'default' => 'date',
+		),
+		'categories'      => array(
+			'type' => 'string',


Minor: Alignment on the arrow (for consistency at least)

aduth · 2017-10-30T12:55:15Z

blocks/query-panel/index.js

+					value: 'title/asc',
+				},
+				{
+					label: __( 'Z → A' ),


Trying to think through localization impact here; would this be translated differently by language? We might consider at least including a translator comment to explain the intent here:

https://codex.wordpress.org/I18n_for_WordPress_Developers#Descriptions

aduth · 2017-10-30T12:56:47Z

blocks/query-panel/index.js

+	onOrderChange,
+	onOrderByChange,
+} ) {
+	return ( <div>


For styling consistency I'd recommend not ending lines with the opening tag of an element and rather having these on their own line. Especially in the case of the line below with <SelectControl it helps readability to associate props with the opening tag and not losing context with the condition in which it renders.

return ( <div> { ( onOrderChange || onOrderByChange ) && ( <SelectControl label={ __( 'Order by' ) }

aduth · 2017-10-30T12:58:33Z

blocks/query-panel/index.js

+			onChange={ ( value ) => {
+				const [ newOrderBy, newOrder ] = value.split( '/' );
+				if ( newOrder !== order ) {
+					( onOrderChange || noop )( newOrder );


We could probably test truthiness of the callback in the condition on the line above and avoid the dependency on noop. This also reads a little "clever".

aduth · 2017-10-30T13:01:51Z

blocks/term-tree-select/index.js

+import SelectControl from '../inspector-controls/select-control';
+
+function getSelectOptions( terms, level = 0 ) {
+	return reduce( terms, ( options, term ) => (


You might consider Lodash's flatMap which I recently found in #3185 to perform much better than Array#reduce + Array#concat.

Further, we can use array spread syntax to simplify this a bit:

function getSelectOptions( terms, level = 0 ) { return flatMap( terms, ( term ) => [ { value: term.id, label: repeat( '\u00A0', level * 3 ) + unescapeString( term.name ), }, ...getSelectOptions( term.children, level + 1 ), ] ); }

aduth · 2017-10-30T13:05:08Z

blocks/term-tree-select/index.js

+}
+
+export default function TermTreeSelect( { termsTree, label, noOptionLabel, selectedTerm, onChange } ) {
+	const options = ( noOptionLabel ? [ { value: '', label: noOptionLabel } ] : [] ).concat( getSelectOptions( termsTree ) );


This is a long line and difficult to read, I would suggest breaking it down a bit.

const options = ( noOptionLabel ? [ { value: '', label: noOptionLabel } ] : [] ).concat( getSelectOptions( termsTree ) );

You could also use Lodash's compact to drop falsey values:

const options = compact( [ noOptionLabel && { value: '', label: noOptionLabel }, ...getSelectOptions( termsTree ), ] );

aduth · 2017-10-30T13:06:05Z

editor/utils/terms.js

@@ -0,0 +1,27 @@
+/**


Can we get some unit tests for this?

Added :) they were missing at the time because I was not certain if a utils function was right for this case.

jorgefilipecosta · 2017-10-31T23:51:46Z

Thank you for your suggestions @aduth. I think I addressed all of them excluding an alignment in a php file. I made some changes to make things look better, but type item in categories is still not aligned with other types of other attributes, as the other type elements are not siblings (they are "cousins") if we add more spaces we break a phpcs rule. Regarding the refactoring to withAPIData in latest posts, as postsCollection.fetch supported exactly the parameters we needed, I would prefer to do this change in a separate PR.

aduth · 2017-11-07T21:14:13Z

blocks/library/latest-posts/index.js

-			const { setAttributes } = this.props;
-
-			if ( postToShowCurrent === postToShowNext ) {
+			if ( this.props.attributes === nextProps.attributes ) {


I don't think we want to rely on a strict equality check here; these two objects may in fact be different references while containing the same values for postsToShow, order, orderBy, categories. If we need to, we can use Lodash's _.isEqual for a deep equality comparison.

aduth · 2017-11-07T21:15:26Z

blocks/query-panel/category-select.js

+	categories: '/wp/v2/categories',
+} ) );
+
+export default flowRight( [


The flowRight here is unnecessary at this point (with a single higher-order component) and could be written as:

export default applyWithAPIData( CategorySelect );

aduth · 2017-11-07T21:16:50Z

blocks/query-panel/index.js

+	onOrderByChange,
+} ) {
+	return (
+		<div>


If the div is not serving a purpose on its own, wondering if we ought to just return as an array, with the only gotcha being we'd have to add keys to each rendered element, but this would reduce the total number of DOM nodes on the screen and reduce indentation in the component:

return [ <SelectControl key="..." // ... ];

jorgefilipecosta · 2017-11-08T19:30:51Z

Hi @aduth, thank you for your feedback 👍 , I rebased and addressed all the points.

aduth

Requests incurred from unrelated attribute changes should be addressed.

aduth · 2017-11-09T20:25:39Z

blocks/library/latest-posts/index.php

 	'render_callback' => 'gutenberg_render_block_core_latest_posts',
-) );
+	'attributes'      =>
+		array(


Why was this changed to put array opening on a line of its own? This isn't a style I've seen common to WordPress, and adds quite a bit of excessive indentation and awkward arrow spacing.

It was a try to make things look more aligned while respecting phpcs rules. I reverted this change but unfortunately, I'm not seeing a way to make things more aligned and respect phpcs.

aduth · 2017-11-09T20:26:56Z

blocks/library/latest-posts/index.js

-			const { setAttributes } = this.props;
-
-			if ( postToShowCurrent === postToShowNext ) {
+			if ( isEqual( this.props.attributes, nextProps.attributes ) ) {


Just noting that we really want to check that a change in the attributes specific to the fetching of posts are the only ones we want to trigger a new request for; as implemented, a change to the block's alignment, columns, layout will trigger a new request.

aduth · 2017-11-09T20:28:41Z

blocks/query-panel/index.js

+					},
+					{
+
+						/* translators: label for ordering posts by title in ascending order, translation may be required for languages using non latin alphabet */


Kinda feel we can drop the rest of the comment after the first comma, that's really up to the translator to judge for themselves, and the first bit should be sufficient for understanding the intent of the string.

Personal preference would be to drop the newline prior to this comment, feels in line with this ESLint rule we have enabled:

https://eslint.org/docs/rules/padded-blocks (never)

aduth · 2017-11-09T20:30:44Z

blocks/query-panel/index.js

+				onChange={ onCategoryChange }
+			/> ),
+		onNumberOfItemsChange &&
+				(


Inconsistency here with the previous condition, and also tabbed at least once more than necessary. Would suggest:

onNumberOfItemsChange && ( <RangeControl key="query-panel-range-control" label={ __( 'Number of items' ) } value={ numberOfItems } onChange={ onNumberOfItemsChange } min={ minItems } max={ maxItems } /> ),

karmatosed

Design wise approving. I did find a bug though in changing the number of items. It got stuck after going to higher number then 1.

codecov · 2017-11-13T23:48:33Z

Codecov Report

Merging #3198 into master will decrease coverage by 0.2%.
The diff coverage is 26.47%.

@@            Coverage Diff             @@
##           master    #3198      +/-   ##
==========================================
- Coverage   37.85%   37.65%   -0.21%     
==========================================
  Files         279      283       +4     
  Lines        6778     6748      -30     
  Branches     1240     1227      -13     
==========================================
- Hits         2566     2541      -25     
+ Misses       3547     3545       -2     
+ Partials      665      662       -3

Impacted Files	Coverage Δ
blocks/library/latest-posts/data.js	`0% <ø> (ø)`	⬆️
blocks/term-tree-select/index.js	`0% <0%> (ø)`
blocks/library/latest-posts/index.js	`6.38% <0%> (-3.43%)`	⬇️
utils/terms.js	`100% <100%> (ø)`
blocks/query-panel/index.js	`25% <25%> (ø)`
blocks/query-panel/category-select.js	`33.33% <33.33%> (ø)`
components/panel/color.js	`0% <0%> (-100%)`	⬇️
components/panel/row.js	`0% <0%> (-100%)`	⬇️
editor/components/warning/index.js	`0% <0%> (-100%)`	⬇️
editor/utils/mobile/index.js	`57.14% <0%> (-17.86%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d37adf...e17bef4. Read the comment docs.

jorgefilipecosta · 2017-11-14T00:05:54Z

Hi @karmatosed, I was able to replicate something similar to what you described. When increasing number of posts to show the posts were not refreshing, leaving the user to be stuck in one post if a value of one was set before. It was introduced by a typo I made while addressing the last reviews and was fixed. Thank you for reporting the problem and for reviewing the design :)

mcsf

I need to run, so here's a more drive-by review.

mcsf · 2017-11-24T16:23:39Z

blocks/library/latest-posts/index.js

+			if ( isEqual(
+				pick( this.props.attributes, attrsTriggerRefresh ),
+				pick( nextProps.attributes, attrsTriggerRefresh )
+			) ) {


Personally not a fan of the name attrsTriggerRefresh vs. e.g. queryKeys/queryProps. Though the name could be skipped altogether:

const shouldRefetch = [ 'a', 'b' ].some( ( queryKey ) => this.props.attributes[ queryKey ] !== nextProps.attributes[ queryKey ] );

(or the converse with shouldSkip and Array#every)

mcsf · 2017-11-24T17:56:47Z

editor/utils/terms.js

+	};
+
+	const ret = fillWithChildren( termsByParent[ 0 ] || [] );
+	return ret;


Leftover assignment of ret?

mcsf · 2017-11-24T17:57:35Z

blocks/query-panel/category-select.js

+	return (
+		<TermTreeSelect
+			{ ...{ label, noOptionLabel } }
+			termsTree={ buildTermsTree( categories.data ) }


What happens while we're waiting for the data request? Should we do an early null return?

mcsf · 2017-11-24T18:08:04Z

blocks/term-tree-select/index.js

+ */
+import { unescape as unescapeString, repeat, flatMap, compact } from 'lodash';
+
+import SelectControl from '../inspector-controls/select-control';


Missing Internal dependencies comment (also, if you want to be really consistent, the External one should use lowercase d 😅 ).

mcsf · 2017-11-24T18:11:05Z

blocks/query-panel/category-select.js

+function CategorySelect( { label, noOptionLabel, categories, selectedCategory, onChange } ) {
+	return (
+		<TermTreeSelect
+			{ ...{ label, noOptionLabel } }


Since you have this spread, you can throw in onChange.

mcsf · 2017-11-24T18:16:54Z

blocks/library/latest-posts/index.php

@@ -62,8 +65,12 @@ function gutenberg_render_block_core_latest_posts( $attributes ) {
 	return $block_content;
 }

-register_block_type( 'core/latest-posts', array(
+register_block_type('core/latest-posts', array(


Should restore that space before the string 'core/latest-posts'.

mcsf · 2017-11-24T18:57:16Z

blocks/query-panel/index.js

+					if ( newOrder !== order && 'function' === typeof onOrderChange ) {
+						onOrderChange( newOrder );
+					}
+					if ( newOrderBy !== orderBy && 'function' === typeof onOrderByChange ) {


If onOrderChange and onOrderByChange default to Lodash's noop, the function checks here can be dropped.

mcsf · 2017-11-24T19:28:58Z

editor/utils/terms.js

+		} );
+	};
+
+	const ret = fillWithChildren( termsByParent[ 0 ] || [] );


Would termsByParent[ '0' ] be more correct? Since it's dict key.

jorgefilipecosta · 2017-11-27T21:56:25Z

Hi @mcsf, thank you a lot for your review, your feedback was applied.

mcsf · 2017-11-28T13:28:16Z

editor/utils/test/terms.js

+import { buildTermsTree } from '../terms';
+
+describe( 'buildTermsTree()', () => {
+	describe( 'isHorizontalEdge', () => {


Some copy-paste that went wrong? 😄

Yes @mcsf I missed it, it was solved :D

mcsf · 2017-11-30T13:42:09Z

blocks/library/latest-posts/data.js

@@ -2,15 +2,22 @@
 * Returns a Promise with the latest posts or an error on failure.
 *
 * @param   {Number} postsToShow       Number of posts to display.
+ * @param   {String} order             Sort attribute ascending or descending.


I recommend enumerating the two options: desc, asc and rephrasing to something like: Whether to sort in ascending or descending order: 'asc'|'desc'.

mcsf · 2017-11-30T13:44:47Z

blocks/library/latest-posts/data.js

@@ -2,15 +2,22 @@
 * Returns a Promise with the latest posts or an error on failure.
 *
 * @param   {Number} postsToShow       Number of posts to display.
+ * @param   {String} order             Sort attribute ascending or descending.
+ * @param   {String} orderBy           Attribute specifying how to sort the posts.


Post parameter by which to sort posts.

mcsf · 2017-11-30T15:27:35Z

blocks/query-panel/category-select.js

+/**
+ * Internal dependencies
+ */
+import { buildTermsTree } from '../../editor/utils/terms';


buildTermsTree is in @wordpress/editor, this should be a WordPress dependency.

Moved to @wordpress/utils :)

Good catch @mcsf, blocks library shouldn't depend on the editor. 👍

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Oct 27, 2017

jorgefilipecosta force-pushed the add/query-panel branch 2 times, most recently from 1ccd40e to fc9e2da Compare October 27, 2017 11:53

jorgefilipecosta changed the title ~~Implemented query panel in latest posts block.~~ WIP Implemented query panel in latest posts block. Oct 27, 2017

jorgefilipecosta self-assigned this Oct 27, 2017

jorgefilipecosta force-pushed the add/query-panel branch 5 times, most recently from d6618c4 to f11d90e Compare October 27, 2017 15:35

jorgefilipecosta requested review from youknowriad and ellatrix October 27, 2017 15:49

jorgefilipecosta changed the title ~~WIP Implemented query panel in latest posts block.~~ Implemented query panel in latest posts block. Oct 27, 2017

jorgefilipecosta requested a review from karmatosed October 27, 2017 15:50

aduth reviewed Oct 30, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch 7 times, most recently from 2290356 to dcb3e7a Compare October 31, 2017 22:43

jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Nov 1, 2017

aduth reviewed Nov 7, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch from dcb3e7a to 521f28b Compare November 8, 2017 16:08

aduth requested changes Nov 9, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch 2 times, most recently from 448d807 to 3109a67 Compare November 10, 2017 18:46

karmatosed requested changes Nov 13, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch from 3109a67 to 8aacd1a Compare November 13, 2017 23:40

jorgefilipecosta requested review from mcsf and gziolo November 22, 2017 21:28

mcsf reviewed Nov 24, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch 3 times, most recently from 5f30b3e to b4e7465 Compare November 27, 2017 21:42

mcsf reviewed Nov 28, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch 3 times, most recently from eebf4c0 to dfe86f8 Compare November 28, 2017 16:17

mcsf reviewed Nov 30, 2017

View reviewed changes

jorgefilipecosta force-pushed the add/query-panel branch 3 times, most recently from 988ae68 to 81189ae Compare November 30, 2017 17:56

jorgefilipecosta force-pushed the add/query-panel branch from 81189ae to e17bef4 Compare December 7, 2017 18:38

Implemented query panel in latest posts block.

ee07318

jorgefilipecosta force-pushed the add/query-panel branch from e17bef4 to ee07318 Compare December 13, 2017 23:29

jorgefilipecosta merged commit d94d742 into master Dec 14, 2017

jorgefilipecosta deleted the add/query-panel branch December 14, 2017 10:08

jorgefilipecosta mentioned this pull request Dec 21, 2017

Query Panel Design / API (Latest Posts, Calendar, etc) #2662

Closed

draganescu mentioned this pull request Feb 5, 2020

[ Latest Posts ] Iterations to version 2 #20046

Closed

Implemented query panel in latest posts block. #3198

Implemented query panel in latest posts block. #3198

Conversation

jorgefilipecosta commented Oct 27, 2017

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Notes

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 8, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2017 • edited Loading

Codecov Report

jorgefilipecosta commented Nov 14, 2017

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 30, 2017 •

edited

Loading

jorgefilipecosta commented Oct 31, 2017 •

edited

Loading

codecov bot commented Nov 13, 2017 •

edited

Loading

jorgefilipecosta commented Nov 27, 2017 •

edited

Loading