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

Jump to bottom

Effects: Move trash post URL change to the BrowserUrl component #7228

Merged
merged 5 commits into from
Jun 20, 2018
Merged

Effects: Move trash post URL change to the BrowserUrl component #7228

merged 5 commits into from
Jun 20, 2018

Conversation

youknowriad
Copy link
Contributor

Related #7096

This PR consolidates all browser navigation (url changes and actual navigation) in the BrowserUrl component of the edit-post module making.

Testing instructions

  • Click "Trash Post"
  • Once the removal succeeds, you should be redirected to the post list.

@youknowriad youknowriad requested a review from aduth June 8, 2018 14:11
@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Code Quality Issues or PRs that relate to code quality labels Jun 8, 2018
@youknowriad youknowriad force-pushed the update/consolidate-post-trash-url-change branch 2 times, most recently from 80907ae to f0f1995 Compare June 8, 2018 14:36
@mkaz
Copy link
Member

mkaz commented Jun 8, 2018

👍 Fixes #7136 - at least give us the ability we need to fix.

@youknowriad youknowriad force-pushed the update/consolidate-post-trash-url-change branch from f0f1995 to 3337f28 Compare June 8, 2018 14:47
aduth
aduth reviewed Jun 8, 2018
View reviewed changes
editor/store/effects.js Outdated
@@ -254,6 +253,8 @@ export default {
dispatch( removeNotice( TRASH_POST_NOTICE_ID ) );
apiRequest( { path: `/wp/v2/${ basePath }/${ postId }`, method: 'DELETE' } ).then(
() => {
const post = getCurrentPost( getState() );
dispatch( resetPost( { ...post, status: 'trashed' } ) );
Copy link
Member

Choose a reason for hiding this comment

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

The correct status is 'trash'.

https://codex.wordpress.org/Function_Reference/get_post_status#Return_Values

editor/store/effects.js Outdated
@@ -254,6 +253,8 @@ export default {
dispatch( removeNotice( TRASH_POST_NOTICE_ID ) );
apiRequest( { path: `/wp/v2/${ basePath }/${ postId }`, method: 'DELETE' } ).then(
() => {
const post = getCurrentPost( getState() );
dispatch( resetPost( { ...post, status: 'trashed' } ) );
Copy link
Member

Choose a reason for hiding this comment

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

This seems like what updatePost is meant to handle (updating subsets of post properties). Unfortunately we have it tangled with change detection so that it considers dirtiness as reset. This might not be a problem if we expect a redirect to occur anyways (in fact maybe a good thing to avoid any possibility of a prompt).

Related: #7130 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

This seems like what updatePost is meant to handle [...]

What do you think about this? Should we try to use updatePost? If it's at least the desirable end-solution, should we add a "TODO" comment or task to follow-up on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be a todo for now because at some point I want to move the currentPost out of the editor's state into core-data, in which case we'd have a updateEntityRecord action

edit-post/components/browser-url/index.js
*
* @return {string} Post trashed URL.
*/
export function getPostTrashedURL( postId, postType ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we export? For tests? 😉

edit-post/components/browser-url/index.js Outdated
const { historyId } = this.state;

if ( postStatus === 'trashed' ) {
Copy link
Member

Choose a reason for hiding this comment

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

The logic flow for this lifecycle function is a bit confusing, since it handles 2 cases (with 3 conditions including a return) for which one but not the other is delegated to a separate function (setBrowserURL).

Should we consolidate to two conditions (one per case), and use separate functions consistently?

componentDidUpdate( prevProps ) {
	const { postId, postStatus, postType } = this.props;

	if ( postStatus === 'trashed' ) {
		this.setTrashURL( postId, postType );
	}

	const { historyId } = this.state;
	if ( ( postId !== prevProps.postId || postId !== historyId ) && postStatus !== 'auto-draft' ) {
		this.setBrowserURL( postId );
	}
}

Or maybe some other pattern for handling multiple types of lifecycle changes, e.g. componentDidUpdate assigned by _.over or _.cond.

Similar to:

gutenberg/editor/components/navigable-toolbar/index.js

Lines 33 to 35 in 5d7bc40

this.switchOnKeyDown = cond( [
[ matchesProperty( [ 'keyCode' ], ESCAPE ), this.focusSelection ],
] );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the less smart option :P. I think it's a little bit easier to understand.

@youknowriad youknowriad force-pushed the update/consolidate-post-trash-url-change branch from 63a88bd to c9829bc Compare June 11, 2018 10:44
@youknowriad
Copy link
Contributor Author

Can I get another review on this one? It should be ready

aduth
aduth approved these changes Jun 19, 2018
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

editor/store/effects.js Outdated
@@ -254,6 +253,8 @@ export default {
dispatch( removeNotice( TRASH_POST_NOTICE_ID ) );
apiRequest( { path: `/wp/v2/${ basePath }/${ postId }`, method: 'DELETE' } ).then(
() => {
const post = getCurrentPost( getState() );
dispatch( resetPost( { ...post, status: 'trash' } ) );
dispatch( {
...action,
type: 'TRASH_POST_SUCCESS',
Copy link
Member

Choose a reason for hiding this comment

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

This action type is never referenced elsewhere. Should we remove it? My only hesitation might be is that it's complementing the failing type, which is in-fact referenced. The counter-point may be that the existence of this action type may be misinterpreted as a hint that it has resulting handling. I'd lean toward removal.

Aside: I'd also like to refactor the notices behavior, which would include consolidating the TRASH_POST_FAILURE action type.

editor/store/effects.js Outdated
@@ -254,6 +253,8 @@ export default {
dispatch( removeNotice( TRASH_POST_NOTICE_ID ) );
apiRequest( { path: `/wp/v2/${ basePath }/${ postId }`, method: 'DELETE' } ).then(
() => {
const post = getCurrentPost( getState() );
dispatch( resetPost( { ...post, status: 'trash' } ) );
Copy link
Member

Choose a reason for hiding this comment

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

We should make a note about updating this to updatePost so it's not overlooked (issue or inline comment).

@aduth aduth mentioned this pull request Jun 19, 2018
Closed
@youknowriad youknowriad merged commit 773e37f into master Jun 20, 2018
@youknowriad youknowriad deleted the update/consolidate-post-trash-url-change branch June 20, 2018 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

Assignees
No one assigned
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@youknowriad @mkaz @aduth