Editor: Added slug selector #4802

abotteram · 2018-02-01T09:49:47Z

Description

Adds selector for slug.

How Has This Been Tested?

Added unit tests.
Ran wp.data.select( "core/editor", "getCurrentPostSlug" ); in the console.

Types of changes

Addition to the selector API.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

atimmer · 2018-02-01T11:33:52Z

Will this work together with #3418? We might have to use getEditedPostAttribute.

abotteram · 2018-02-01T11:43:54Z

@atimmer Makes sense, refactored.

…tPostEdits to be safer

atimmer

It still needs to be added here: https://github.com/WordPress/gutenberg/blob/master/editor/store/index.js#L29.

Otherwise, it looks good.

gziolo · 2018-02-02T10:38:16Z

editor/store/selectors.js

@@ -200,7 +211,7 @@ export function getCurrentPostLastRevisionId( state ) {
 * @returns {Object} Object of key value pairs comprising unsaved edits.
 */
 export function getPostEdits( state ) {
-	return state.editor.present.edits;
+	return get( state, [ 'editor', 'present', 'edits' ], {} );


Just noting that you can use also string as 2nd param:

return get( state, 'editor.present.edits', {} );

See 3rd example in here

@gziolo I know, this is a small performance consideration because lodash won't have to parse the string.

Hehe, more keystrokes to save some CPU time. They should include lodash into JS engine by default and optimize that for us or teach Babel to do the transformation from string to array!

Just noting that you can use also string as 2nd param:

We seem to be encouraging the opposite thing, as I tend to leave the inverse as a suggestion whenever possible 😉 Beside the performance advantage, I find it trivially more legible by virtue of greater separation between the properties.

Aside: Why would state.editor.present.edits ever be undefined?

(I hope the answer is not "because tests")

Further, if this triggers the default, since it's not a shared reference, two subsequent invocations of this function would not be strictly equal and therefore trigger re-renders on any connected components relying on the result of this function:

const state = {}; getPostEdits( state ) !== getPostEdits( state );

That's a good catch about having {} as the default value. The same would apply to [].

One example where I worked around this was in getBlockOrder:

gutenberg/editor/store/selectors.js

Lines 33 to 40 in e2c824a

/**

* Shared reference to an empty array used as the default block order return

* value when the state value is not explicitly assigned, since we want to

* avoid returning a new array reference on every invocation.

*

* @type {Array}

*/

const DEFAULT_BLOCK_ORDER = [];

gutenberg/editor/store/selectors.js

Lines 788 to 801 in e2c824a

/**

* Returns an array containing all block unique IDs of the post being edited,

* in the order they appear in the post. Optionally accepts a root UID of the

* block list for which the order should be returned, defaulting to the top-

* level block order.

*

* @param {Object} state Global application state.

* @param {?string} rootUID Optional root UID of block list.

*

* @return {Array} Ordered unique IDs of post blocks.

*/

export function getBlockOrder( state, rootUID ) {

return state.editor.present.blockOrder[ rootUID || '' ] || DEFAULT_BLOCK_ORDER;

}

I'd not love if we start having many constants at the top of this file, but we also should try to avoid these fallbacks whenever possible (instead relying on the value reference living in state as the result of the reducer).

There could be 2 constants, one for an empty array and one for an empty object. However I second your opinion that reducers should handle it.

gziolo · 2018-02-02T10:39:00Z

editor/store/selectors.js

@@ -214,9 +225,10 @@ export function getPostEdits( state ) {
 * @returns {*} Post attribute value.
 */
 export function getEditedPostAttribute( state, attributeName ) {
-	return state.editor.present.edits[ attributeName ] === undefined ?
+	const edits = getPostEdits( state );


gziolo · 2018-02-02T10:41:47Z

editor/store/test/selectors.js

@@ -375,6 +376,31 @@ describe( 'selectors', () => {
 		} );
 	} );

+	describe( 'getCurrentPostSlug', () => {
+		it( 'should return the current post\'s slug is no edits have been made', () => {


typo: slug *is* no edits -> slug if no edits

gziolo · 2018-02-02T10:46:10Z

Looks good, nice work on this one. I would wait for @youknowriad to perform a final check. I'm still learning how this should be exposed to make it scalable. We might want also to document all exposed selectors in our docs.

youknowriad · 2018-02-02T10:57:34Z

👍 LGTM Also @aduth proposed that we expose all our selectors (which forces us to ensure backwards compatibilty but maybe it's fine)

aduth

Why do we need a dedicated selector for slug, when getEditedPostAttribute should work just as well?

I'd like to avoid excessive proliferation of selectors, since as implemented with these changes, there's no reason we shouldn't have selectors for the 23 other properties of the edited post (I'm arguing this as a bad thing).

aduth · 2018-02-08T00:41:27Z

editor/store/selectors.js

@@ -200,7 +211,7 @@ export function getCurrentPostLastRevisionId( state ) {
 * @returns {Object} Object of key value pairs comprising unsaved edits.
 */
 export function getPostEdits( state ) {
-	return state.editor.present.edits;
+	return get( state, [ 'editor', 'present', 'edits' ], {} );


Just noting that you can use also string as 2nd param:

We seem to be encouraging the opposite thing, as I tend to leave the inverse as a suggestion whenever possible 😉 Beside the performance advantage, I find it trivially more legible by virtue of greater separation between the properties.

aduth · 2018-02-08T00:42:31Z

editor/store/selectors.js

@@ -200,7 +211,7 @@ export function getCurrentPostLastRevisionId( state ) {
 * @returns {Object} Object of key value pairs comprising unsaved edits.
 */
 export function getPostEdits( state ) {
-	return state.editor.present.edits;
+	return get( state, [ 'editor', 'present', 'edits' ], {} );


Aside: Why would state.editor.present.edits ever be undefined?

(I hope the answer is not "because tests")

aduth · 2018-02-08T00:43:37Z

editor/store/selectors.js

@@ -200,7 +211,7 @@ export function getCurrentPostLastRevisionId( state ) {
 * @returns {Object} Object of key value pairs comprising unsaved edits.
 */
 export function getPostEdits( state ) {
-	return state.editor.present.edits;
+	return get( state, [ 'editor', 'present', 'edits' ], {} );


Further, if this triggers the default, since it's not a shared reference, two subsequent invocations of this function would not be strictly equal and therefore trigger re-renders on any connected components relying on the result of this function:

const state = {}; getPostEdits( state ) !== getPostEdits( state );

abotteram · 2018-02-08T08:55:36Z

@aduth I'm starting work on addressing your concerns.

Added slug selector

1c0b781

Refactored slug selector to use getEditedPostAttribute, refactored ge…

1b02fec

…tPostEdits to be safer

atimmer requested a review from youknowriad February 1, 2018 14:28

atimmer approved these changes Feb 1, 2018

View reviewed changes

added getCurrentPostSlug selector to data API

2e585bb

gziolo reviewed Feb 2, 2018

View reviewed changes

gziolo approved these changes Feb 2, 2018

View reviewed changes

gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 2, 2018

gziolo assigned abotteram Feb 2, 2018

Fixed spelling

a530eb6

gziolo changed the title ~~Added slug selector~~ Data: Added slug selector Feb 2, 2018

gziolo changed the title ~~Data: Added slug selector~~ Editor: Added slug selector Feb 2, 2018

gziolo merged commit ef95c64 into WordPress:master Feb 2, 2018

atimmer deleted the add/slug-selector branch February 2, 2018 14:25

aduth reviewed Feb 8, 2018

View reviewed changes

abotteram mentioned this pull request Feb 12, 2018

Expose getEditedPostAttribute selector #5003

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Added slug selector #4802

Editor: Added slug selector #4802

abotteram commented Feb 1, 2018 •

edited

Loading

atimmer commented Feb 1, 2018 •

edited

Loading

abotteram commented Feb 1, 2018 •

edited

Loading

atimmer left a comment

gziolo Feb 2, 2018

abotteram Feb 2, 2018

gziolo Feb 2, 2018

aduth Feb 8, 2018

aduth Feb 8, 2018

aduth Feb 8, 2018

gziolo Feb 8, 2018

aduth Feb 8, 2018

gziolo Feb 8, 2018

gziolo Feb 2, 2018

gziolo Feb 2, 2018 •

edited

Loading

gziolo commented Feb 2, 2018

youknowriad commented Feb 2, 2018

aduth left a comment

aduth Feb 8, 2018

aduth Feb 8, 2018

aduth Feb 8, 2018

abotteram commented Feb 8, 2018

	/**
	* Shared reference to an empty array used as the default block order return
	* value when the state value is not explicitly assigned, since we want to
	* avoid returning a new array reference on every invocation.
	*
	* @type {Array}
	*/
	const DEFAULT_BLOCK_ORDER = [];

	/**
	* Returns an array containing all block unique IDs of the post being edited,
	* in the order they appear in the post. Optionally accepts a root UID of the
	* block list for which the order should be returned, defaulting to the top-
	* level block order.
	*
	* @param {Object} state Global application state.
	* @param {?string} rootUID Optional root UID of block list.
	*
	* @return {Array} Ordered unique IDs of post blocks.
	*/
	export function getBlockOrder( state, rootUID ) {
	return state.editor.present.blockOrder[ rootUID \|\| '' ] \|\| DEFAULT_BLOCK_ORDER;
	}

Editor: Added slug selector #4802

Editor: Added slug selector #4802

Conversation

abotteram commented Feb 1, 2018 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

atimmer commented Feb 1, 2018 • edited Loading

abotteram commented Feb 1, 2018 • edited Loading

atimmer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo commented Feb 2, 2018

youknowriad commented Feb 2, 2018

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

abotteram commented Feb 8, 2018

abotteram commented Feb 1, 2018 •

edited

Loading

atimmer commented Feb 1, 2018 •

edited

Loading

abotteram commented Feb 1, 2018 •

edited

Loading

gziolo Feb 2, 2018 •

edited

Loading