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.

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

Post Editor: Refactor URL redirect as BrowserURL component #7122

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 4, 2018

Related: #7067

This pull request seeks to resolve an issue where the browser URL does not change when a new post is autosaved. This was partly introduced by #7067, which was intended to address the case where an autosave has its own ID which should not be used in place of that of the saved post.

These changes are a refactoring of the URL replacement logic, moving from editor to edit-post and expressing in terms of application UI state lifecycle as: Sync to current (saved) post ID so long as post is not auto-draft.

In doing so, it also implements an improvement to the state.currentPost reducer, reflecting the fact that when an autosave occurs, the saved post can no longer be considered auto-draft (it automatically changes to draft during server-side save logic).

Testing instructions:

Verify that, when editing a new post, the URL remains post-new.php

Verify that, after autosave kicks in for a new post, the URL changes to one with an ID

Repeat testing instructions from #7067

aduth added 2 commits June 4, 2018 11:26
Behavior is specific to post editor, and can be expressed in terms of lifecycle as: Sync to current post ID so long as post is not auto-draft.
@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jun 4, 2018
@aduth aduth added this to the 3.0 milestone Jun 4, 2018
'Post ' + post.id,
getPostEditUrl( post.id )
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I love that this moves the URL change to a "side-effect" outside the editor module. Nice work


export default withSelect( ( select ) => {
const { getCurrentPost } = select( 'core/editor' );
const { id, status } = getCurrentPost();
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the current post is updated when a request triggers (and not when it succeed). Could this be an issue if the request fails and the status is reverted for instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could this be an issue if the request fails and the status is reverted for instance?

Hmm, I suppose this could lead to some oddity where the UI is too optimistic. It might be reproduceable for a new post where the status is explicitly set to something. When hitting save, I expect the URL will change immediately. If the request then fails, the status would be reverted back to auto-draft but, since we don't allow replaceState to be called for auto-draft, it will linger in its optimistic state.

Might be worth discussing more what would actually be expected here. But, for what it's worth, in my brief testing I find that navigating to post.php?id=XYZ where XYZ is the ID of an auto-draft post, the editor loads just fine (i.e. no effective breakage).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'm not too concerned here as well. I guess the alternative would be to revert the url to post-new if the status is reverted to auto-draft

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm not too concerned here as well. I guess the alternative would be to revert the url to post-new if the status is reverted to auto-draft

Yep, sounds reasonable. Since it's not breaking, I'll create an issue and we can address this after release.

@@ -559,6 +559,16 @@ export function currentPost( state = {}, action ) {
}

return mapValues( post, getPostRawValue );

case 'RESET_AUTOSAVE':
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this surfaces another bug (or not, depends on the interpretation :) ) about autosaves. On draft posts, auto-save update the draft itself, so when you refresh the page you get the auto-saved version which is different from the way it works for published posts.

Maybe that's the intent but in that case maybe we shouldn't consider the post as dirty anymore if it's auto saved when it's a draft. Maybe auto-saving a draft is not an auto-save but just a regular save.

For me, it's fine to address later.

Copy link
Member Author

Choose a reason for hiding this comment

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

On draft posts, auto-save update the draft itself

Yep, I noticed this too. Specifically, we should be accounting for this server-side logic in the client as well:

if ( ( 'draft' === $post->post_status || 'auto-draft' === $post->post_status ) && $post->post_author == $user_id ) {
// Draft posts for the same author: autosaving updates the post and does not create a revision.
// Convert the post object to an array and add slashes, wp_update_post expects escaped array.
$autosave_id = wp_update_post( wp_slash( (array) $prepared_post ), true );

I do think this is a separate issue though.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Still some small concerns about how draft auto-saves work but It's unrelated to this change.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants