Use getEditedPostAttribute() in more selectors
#6894
Conversation
|
@gziolo Notably, four tests fail with these changes. The problem is that |
| @@ -353,7 +354,7 @@ export function getEditedPostExcerpt( state ) { | |||
| * @return {string} Preview URL. | |||
| */ | |||
| export function getEditedPostPreviewLink( state ) { | |||
| return getCurrentPost( state ).preview_link || null; | |||
| return getEditedPostAttribute( state, 'preview_link' ); | |||
There was a problem hiding this comment.
I think you can use getEditedPostAttribute directly to avoid exposing another selector.
There was a problem hiding this comment.
Oh. I get what you're saying. getEditedPostPreviewLink() already existed in #6882 though. What's our policy on removing selectors?
|
Thanks for opening this PR, it would be nice to get it reviewed by @youknowriad or @aduth to make sure we introduce proper changes. I was mostly concerned about introducing another selector |
|
Just so it's stated, I'm not necessarily sure that the changes in this PR are a good thing. I wanted to open it up for illustration purpose. |
| @@ -76,7 +76,7 @@ export function hasEditorRedo( state ) { | |||
| * @return {boolean} Whether the post is new. | |||
| */ | |||
| export function isEditedPostNew( state ) { | |||
| return getCurrentPost( state ).status === 'auto-draft'; | |||
| return getEditedPostAttribute( state, 'status' ) === 'auto-draft'; | |||
There was a problem hiding this comment.
I'm not certain about this change. Because getEditedPostAttribute checks the edited values as well. So if someone sets the status to "draft" or something else and diidn't save yet. There will be a difference. The post won't be considered new while it's still new.
| @@ -227,7 +227,7 @@ export function getEditedPostVisibility( state ) { | |||
| * @return {boolean} Whether current post is pending review. | |||
| */ | |||
| export function isCurrentPostPending( state ) { | |||
| return getCurrentPost( state ).status === 'pending'; | |||
| return getEditedPostAttribute( state, 'pending' ) === 'pending'; | |||
| @@ -238,10 +238,11 @@ export function isCurrentPostPending( state ) { | |||
| * @return {boolean} Whether the post has been published. | |||
| */ | |||
| export function isCurrentPostPublished( state ) { | |||
| const post = getCurrentPost( state ); | |||
| const status = getEditedPostAttribute( state, 'status' ); | |||
There was a problem hiding this comment.
II also believe we wanted to get the current post's status and date here and not the potentially updated ones.
|
So basically, I think we shouldn't change the selectors here because there's a difference between the "current post" (which means the last saved version) and the "editted attributes" which are potentially different. I think the idea of using this |
|
Ok, closing for now. |
|
Already stated, the difference is when we want to operate on the canonical saved value vs. an unsaved edit, which is intentional in some selectors ("is post new" based on the saved post status), usually reflected in the name of the selector (
There's some precedent on leaving selectors (or at least arguments behavior) and using |
From #6882 (comment)