Add documentation for every state selector #989

aduth · 2017-06-01T21:15:36Z

This pull request seeks to add documentation for every state selector which exists. Some are a best guess at intent of the selector, so review from original authors would be appreciated.

Testing instructions:

There is no impact on editor experience from these changes.

Ensure that new unit tests pass:

npm run test-unit

nylen

Thanks for doing this. I learned a few things about our state structure already from reviewing it.

nylen · 2017-06-02T00:09:53Z

editor/test/selectors.js

@@ -839,4 +890,57 @@ describe( 'selectors', () => {
 			expect( isSavingNewPost( state ) ).to.be.false();
 		} );
 	} );
+
+	describe( 'getSuggestedPostFormat', () => {
+		it( 'returns null if cannot be determined', () => {


Wording of these test names? Most of our other tests use should, and it seems good to be consistent...

it( 'should be painted yellow', ...

Wording of these test names?

We just talked about this at #946 (comment) 😛 I'd argue this is one case where I wish we weren't so consistent, because bending over backwards to prefix with "should" isn't always appropriate and at worst is unnecessary filler.

nylen · 2017-06-02T00:11:43Z

editor/test/selectors.js

+				editor: {
+					blockOrder: [ 5, 4, 3, 2, 1 ],
+				},
+				multiSelectedBlocks: { start: 2, end: 4 },


Here and below, it seems strange to have end before start in the block order. Is this possible in actual usage?

Yes, start has the index where the selection started, end where it ended. See also getSelectedBlocks.

(You can select blocks from bottom to top.)

nylen · 2017-06-02T00:12:56Z

editor/selectors.js

 export function isEditorSidebarOpened( state ) {
 	return state.isSidebarOpened;
 }

+/**
+ * Returns true if the past editor history snapshots exist, or false otherwise.


s/the/any/ - it reads better to me this way

nylen · 2017-06-02T00:13:09Z

editor/selectors.js

 export function hasEditorUndo( state ) {
 	return state.editor.history.past.length > 0;
 }

+/**
+ * Returns true if the future editor history snapshots exist, or false


s/the/any/ - it reads better to me this way

nylen · 2017-06-02T00:15:01Z

editor/selectors.js

 export function isEditedPostDirty( state ) {
 	return state.editor.dirty;
 }

+/**
+ * Returns the post currently being edited.


Probably worth noting that this does not include any edits made to the post. "Returns our representation of the post in its last known state state on the server" maybe?

Probably worth noting that this does not include any edits made to the post. "Returns our representation of the post in its last known state state on the server" maybe?

Raises an interesting point about what we'd expect the return value to be for a new post. Based on the currentPost default I'd guess an empty object? Seems like it might be more appropriate as null, though I suspect this could cause breakage where we expect to access properties unguarded on the post object.

Nice thing about adding these documentation is that it helps surface real-world expectations 😄

Not necessarily an empty object - I imagine a new post would contain status: 'draft' and any other default values. Previous discussion: #848 (comment)

Currently these default values are stored in post-content.js (#, #).

So perhaps we could improve the wording here as follows:

Returns our representation of the saved post object. For an existing post, this is the last known state of the post on the server; for a new post, this is an object containing any relevant default values.

nylen · 2017-06-02T00:15:36Z

editor/selectors.js

 export function getCurrentPost( state ) {
 	return state.currentPost;
 }

+/**
+ * Returns the post values which have yet to be saved.


"Returns any post values that have been changed in the editor and not yet saved"?

nylen · 2017-06-02T00:16:18Z

editor/selectors.js

 export function getPostEdits( state ) {
 	return state.editor.edits;
 }

+/**
+ * Returns a single attribute of the post being edited.


"..., pulling from either the last known representation of the post or its edited value, if any."

nylen · 2017-06-02T00:18:58Z

editor/selectors.js

+ * @param  {Object} state Global application state
+ * @param  {String} uid   Block unique ID
+ * @return {Number}       Index at which block exists in order
+ */
 export function getBlockOrder( state, uid ) {


Perhaps separately, I think this selector should be renamed. As-is, I'd expect it to simply return the block order array. getBlockIndex maybe?

nylen · 2017-06-02T00:20:11Z

editor/selectors.js

+/**
+ * Returns focus state of the block corresponding to the specified unique ID,
+ * or null if the block is not selected. Focus state defaults to an empty
+ * object.


What other object shapes might be returned by the focus state?

What other object shapes might be returned by the focus state?

As far as I recall the original intention, it's up to the block to manage the focus object how they see fit. For example differentiating between Editables of the same block:

https://github.com/WordPress/gutenberg/blob/1e46489/blocks/library/quote/index.js#L115

This is one thing I'd like to see refactored, ideally with less block implementer intervention. Relates to #878

cc @youknowriad

Agree this could be improved, for now I would add something like "The specific content of the object is left up to the implementation of each block."

nylen · 2017-06-02T00:20:48Z

editor/selectors.js

@@ -194,6 +430,14 @@ export function isTypingInBlock( state, uid ) {
 	return state.selectedBlock.typing;
 }

+/**
+ * Returns the unique ID of the block at which a new block insertion would be


s/at which/after which/ ?

youknowriad · 2017-06-02T10:07:50Z

editor/selectors.js

+ *
+ * @param  {Object} state Global application state
+ * @return {String}       Post status
+ */
 export function getEditedPostStatus( state ) {


This has been written before the getEditedPostAttribute, I wonder if we should just drop it now.

youknowriad · 2017-06-02T10:09:14Z

editor/selectors.js

 }

+/**
+ * Returns an array containing all unique IDs of the post being edited, in the


all the unique block IDS

aduth · 2017-06-02T15:11:45Z

Revisions made per reviews. Thanks for looking 👍

youknowriad

Thanks for doing this

In favor of direct `getEditedPostAttribute( state, 'status' )`

aduth · 2017-06-02T18:08:32Z

Updated per feedback at #989 (comment) and #989 (comment), squashed the two revisory rewording commits. Will merge when CI passes.

aduth added the [Type] Developer Documentation Documentation for developers label Jun 1, 2017

aduth requested review from youknowriad, mtias and ellatrix June 1, 2017 21:15

nylen reviewed Jun 2, 2017

View reviewed changes

youknowriad reviewed Jun 2, 2017

View reviewed changes

aduth mentioned this pull request Jun 2, 2017

Embed: use 100% width for iframe. #995

Merged

youknowriad approved these changes Jun 2, 2017

View reviewed changes

aduth added 3 commits June 2, 2017 14:05

Document every selector

f34be75

Rename getBlockOrder as getBlockIndex

6f8b9de

Remove getEditedPostStatus selector

db75748

In favor of direct `getEditedPostAttribute( state, 'status' )`

aduth force-pushed the add/selectors-docs branch from b0b11c5 to db75748 Compare June 2, 2017 18:06

aduth merged commit 49ad784 into master Jun 2, 2017

aduth deleted the add/selectors-docs branch June 2, 2017 18:11

snyk-bot mentioned this pull request Feb 1, 2020

[Snyk] Fix for 1 vulnerabilities SeppPenner/gutenberg#114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation for every state selector #989

Add documentation for every state selector #989

aduth commented Jun 1, 2017

nylen left a comment

nylen Jun 2, 2017

aduth Jun 2, 2017

nylen Jun 2, 2017 •

edited

Loading

ellatrix Jun 2, 2017

ellatrix Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

aduth Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

aduth Jun 2, 2017

nylen Jun 2, 2017

nylen Jun 2, 2017

youknowriad Jun 2, 2017

youknowriad Jun 2, 2017

aduth commented Jun 2, 2017

youknowriad left a comment

aduth commented Jun 2, 2017

Add documentation for every state selector #989

Add documentation for every state selector #989

Conversation

aduth commented Jun 1, 2017

nylen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen Jun 2, 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

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

aduth commented Jun 2, 2017

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Jun 2, 2017

nylen Jun 2, 2017 •

edited

Loading