Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor: Added slug selector #4802

Merged
merged 4 commits into from
Feb 2, 2018
Merged

Editor: Added slug selector #4802

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1c0b781
Added slug selector
Feb 1, 2018
1b02fec
Refactored slug selector to use getEditedPostAttribute, refactored ge…
Feb 1, 2018
2e585bb
added getCurrentPostSlug selector to data API
Feb 1, 2018
a530eb6
Fixed spelling
Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions editor/store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
getEditedPostContent,
getEditedPostTitle,
getSelectedBlockCount,
getCurrentPostSlug,
} from './selectors';

/**
Expand All @@ -31,6 +32,7 @@ registerSelectors( MODULE_KEY, {
getEditedPostContent,
getEditedPostTitle,
getSelectedBlockCount,
getCurrentPostSlug,
} );

export default store;
18 changes: 15 additions & 3 deletions editor/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,17 @@ export function getCurrentPostType( state ) {
return state.currentPost.type;
}

/**
* Returns the slug of the post currently being edited.
*
* @param {Object} state Global application state.
*
* @returns {string} Slug.
*/
export function getCurrentPostSlug( state ) {
return getEditedPostAttribute( state, 'slug' );
}

/**
* Returns the ID of the post currently being edited, or null if the post has
* not yet been saved.
Expand Down Expand Up @@ -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' ], {} );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

See 3rd example in here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand All @@ -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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

return edits[ attributeName ] === undefined ?
state.currentPost[ attributeName ] :
state.editor.present.edits[ attributeName ];
edits[ attributeName ];
}

/**
Expand Down
26 changes: 26 additions & 0 deletions editor/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
getCurrentPostLastRevisionId,
getCurrentPostRevisionsCount,
getCurrentPostType,
getCurrentPostSlug,
getPostEdits,
getEditedPostTitle,
getDocumentTitle,
Expand Down Expand Up @@ -375,6 +376,31 @@ describe( 'selectors', () => {
} );
} );

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

@gziolo gziolo Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const state = {
currentPost: { slug: 'post slug' },
};

expect( getCurrentPostSlug( state ) ).toBe( 'post slug' );
} );

it( 'should return the latest slug if edits have been made to the post', () => {
const state = {
currentPost: { slug: 'old slug' },
editor: {
present: {
edits: {
slug: 'new slug',
},
},
},
};

expect( getCurrentPostSlug( state ) ).toBe( 'new slug' );
} );
} );

describe( 'getCurrentPostLastRevisionId', () => {
it( 'should return null if the post has not yet been saved', () => {
const state = {
Expand Down