-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improve autosaves #4156
Improve autosaves #4156
Changes from 60 commits
08cdb15
9d6b95c
1e9572c
b82fa9d
cbd0c62
9374837
218299c
bb7b896
3ee6738
2e3ec2e
86b70c2
db02aea
e82ffc1
9e5ccf8
968241d
52110b7
9997325
f389030
4b31a3c
36e3183
f4f0d47
e484958
4d81b62
15e0573
6bf285c
39fb050
a5b26db
17c26ca
fd6464c
51ac91e
ad4e8b3
9d01248
9dad2ff
471b507
8928813
76e7973
bf03be9
5ba2bf1
ccd6ac6
2e887a2
22cfcfd
96a348b
bf1ab9d
5bf6c7f
a0b5858
4d84074
14147c6
557d653
a988bb8
8d75bb7
389237f
dd2d9b4
3ee7326
8e8e9c0
91a07c0
b2b89ec
ecbf499
19dabd9
7965579
62230fd
2dd8774
765ae5d
06be44d
4bdfad9
c1ef42c
c05796c
95a245b
b7ae85a
8d40fd6
3e559ca
7ace629
12d7225
4d03072
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,20 +23,21 @@ import { | |
isEditedPostSaveable, | ||
getCurrentPost, | ||
getEditedPostAttribute, | ||
isAutosavingPost, | ||
} from '../../store/selectors'; | ||
|
||
export function PostSavedState( { isNew, isPublished, isDirty, isSaving, isSaveable, status, onStatusChange, onSave } ) { | ||
export function PostSavedState ( { isNew, isPublished, isDirty, isSaving, isSaveable, status, onStatusChange, onSave, isAutosaving } ) { | ||
const className = 'editor-post-saved-state'; | ||
|
||
if ( isSaving ) { | ||
return ( | ||
<span className={ className }> | ||
{ __( 'Saving' ) } | ||
{ isAutosaving ? __( 'Autosaving' ) : __( 'Saving' ) } | ||
</span> | ||
); | ||
} | ||
|
||
if ( ! isSaveable || isPublished ) { | ||
if ( ! isSaveable ) { | ||
return null; | ||
} | ||
|
||
|
@@ -59,8 +60,8 @@ export function PostSavedState( { isNew, isPublished, isDirty, isSaving, isSavea | |
|
||
return ( | ||
<Button className={ classnames( className, 'button-link' ) } onClick={ onClick }> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, unless we still want to use this component to show the autosave progress while in action? right now it says 'Autosaving' then 'Saved'. if we do remove this entirely, would disabling/re-enabling the update button be enough to indicate autosaving? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling designers :P @jasmussen @karmatosed As I said, maybe we'd want to separate "autosaving" from "saving drafts" but I'll let our awesome designers find a solution for this. And I don't consider this a blocking for this PR. |
||
<span className="editor-post-saved-state__mobile">{ __( 'Save' ) }</span> | ||
<span className="editor-post-saved-state__desktop">{ __( 'Save Draft' ) }</span> | ||
<span className="editor-post-saved-state__mobile">{ isPublished ? '' : __( 'Save' ) }</span> | ||
<span className="editor-post-saved-state__desktop">{ isPublished ? '' : __( 'Save Draft' ) }</span> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you clarify this change? You are hiding the link only on the desktop. Should we avoid rendering the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, right - i'll fix that. i don't think a save button is appropriate when the post is published as you currently can't save drafts of published posts. i'll add that for mobile. |
||
</Button> | ||
); | ||
} | ||
|
@@ -74,6 +75,7 @@ export default connect( | |
isSaving: isSavingPost( state ), | ||
isSaveable: isEditedPostSaveable( state ), | ||
status: getEditedPostAttribute( state, 'status' ), | ||
isAutosaving: isAutosavingPost( state ), | ||
} ), | ||
{ | ||
onStatusChange: ( status ) => editPost( { status } ), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ import { | |
/** | ||
* Internal Dependencies | ||
*/ | ||
import { setupEditor, undo } from '../../store/actions'; | ||
import { setupEditor, undo, autosaveAlert } from '../../store/actions'; | ||
import store from '../../store'; | ||
|
||
/** | ||
|
@@ -52,6 +52,12 @@ class EditorProvider extends Component { | |
// Assume that we don't need to initialize in the case of an error recovery. | ||
if ( ! props.recovery ) { | ||
this.store.dispatch( setupEditor( props.post, this.settings ) ); | ||
|
||
// @todo only show the autosave alert when there exists an autosave newer than the post | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, leftover There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
// and if that autosave is different than the post (title, excerpt & content) | ||
if ( props.autosave ) { | ||
this.store.dispatch( autosaveAlert( props.autosave ) ); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -220,9 +220,10 @@ export function editPost( edits ) { | |
}; | ||
} | ||
|
||
export function savePost() { | ||
export function savePost( options ) { | ||
return { | ||
type: 'REQUEST_POST_UPDATE', | ||
options, | ||
}; | ||
} | ||
|
||
|
@@ -252,6 +253,20 @@ export function autosave() { | |
}; | ||
} | ||
|
||
export function isAutosaving( isAutosaving ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the actions are better named as "verbs", maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, good suggestion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed |
||
return { | ||
type: 'DOING_AUTOSAVE', | ||
isAutosaving, | ||
} | ||
} | ||
|
||
export function autosaveAlert( autosave ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe better as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, will change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
return { | ||
type: 'REQUEST_AUTOSAVE_EXISTS', | ||
autosave, | ||
} | ||
} | ||
|
||
/** | ||
* Returns an action object used in signalling that undo history should | ||
* restore last popped state. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,10 @@ import { | |
replaceBlocks, | ||
createSuccessNotice, | ||
createErrorNotice, | ||
createWarningNotice, | ||
removeNotice, | ||
savePost, | ||
isAutosaving, | ||
editPost, | ||
requestMetaBoxUpdates, | ||
updateReusableBlock, | ||
|
@@ -44,6 +46,8 @@ import { | |
getDirtyMetaBoxes, | ||
getEditedPostContent, | ||
getPostEdits, | ||
getEditedPostTitle, | ||
getEditedPostExcerpt, | ||
isCurrentPostPublished, | ||
isEditedPostDirty, | ||
isEditedPostNew, | ||
|
@@ -57,6 +61,7 @@ import { | |
* Module Constants | ||
*/ | ||
const SAVE_POST_NOTICE_ID = 'SAVE_POST_NOTICE_ID'; | ||
const AUTOSAVE_POST_NOTICE_ID = 'AUTOSAVE_POST_NOTICE_ID'; | ||
const TRASH_POST_NOTICE_ID = 'TRASH_POST_NOTICE_ID'; | ||
const SAVE_REUSABLE_BLOCK_NOTICE_ID = 'SAVE_REUSABLE_BLOCK_NOTICE_ID'; | ||
|
||
|
@@ -71,26 +76,65 @@ export default { | |
content: getEditedPostContent( state ), | ||
id: post.id, | ||
}; | ||
const isAutosave = action.options && action.options.autosave; | ||
let Model, newModel; | ||
|
||
dispatch( { | ||
type: 'UPDATE_POST', | ||
edits: toSend, | ||
optimist: { type: BEGIN, id: POST_UPDATE_TRANSACTION_ID }, | ||
} ); | ||
dispatch( removeNotice( SAVE_POST_NOTICE_ID ) ); | ||
const Model = wp.api.getPostTypeModel( getCurrentPostType( state ) ); | ||
new Model( toSend ).save().done( ( newPost ) => { | ||
dispatch( { | ||
type: 'RESET_POST', | ||
post: newPost, | ||
} ); | ||
if ( isAutosave ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know I told you to merge the two effects, but I'm starting to regret 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fine to separate them again; as you can see in the code, autosaving is a pretty different action than saving the post - for both drafts and published posts. To summarize the differences:
Also:
|
||
toSend.parent = post.id; | ||
delete toSend.id ; | ||
Model = wp.api.getPostTypeAutosaveModel( getCurrentPostType( state ) ); | ||
newModel = new Model( toSend ); | ||
} else { | ||
dispatch( { | ||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||
previousPost: post, | ||
post: newPost, | ||
optimist: { type: COMMIT, id: POST_UPDATE_TRANSACTION_ID }, | ||
type: 'UPDATE_POST', | ||
edits: toSend, | ||
optimist: { type: BEGIN, id: POST_UPDATE_TRANSACTION_ID }, | ||
} ); | ||
dispatch( removeNotice( SAVE_POST_NOTICE_ID ) ); | ||
dispatch( removeNotice( AUTOSAVE_POST_NOTICE_ID ) ); | ||
Model = wp.api.getPostTypeModel( getCurrentPostType( state ) ); | ||
newModel = new Model( toSend ); | ||
} | ||
|
||
newModel.save().done( ( newPost ) => { | ||
if ( isAutosave ) { | ||
const autosave = { | ||
id: newPost.id, | ||
title: getEditedPostTitle( state ), | ||
excerpt: getEditedPostExcerpt( state ), | ||
content: getEditedPostContent( state ), | ||
}; | ||
dispatch( { | ||
type: 'RESET_AUTOSAVE', | ||
post: autosave, | ||
} ); | ||
dispatch( isAutosaving( false ) ); | ||
|
||
dispatch( { | ||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||
previousPost: post, | ||
post: post, | ||
isAutosave: true, | ||
} ); | ||
} else { | ||
// dispatch post autosaved false | ||
// delete the autosave | ||
dispatch( { | ||
type: 'RESET_POST', | ||
post: newPost, | ||
} ); | ||
dispatch( { | ||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||
previousPost: post, | ||
post: newPost, | ||
optimist: { type: COMMIT, id: POST_UPDATE_TRANSACTION_ID }, | ||
} ); | ||
} | ||
} ).fail( ( err ) => { | ||
if ( isAutosave ) { | ||
dispatch( isAutosaving( false ) ); | ||
} | ||
|
||
dispatch( { | ||
type: 'REQUEST_POST_UPDATE_FAILURE', | ||
error: get( err, 'responseJSON', { | ||
|
@@ -99,12 +143,29 @@ export default { | |
} ), | ||
post, | ||
edits, | ||
optimist: { type: REVERT, id: POST_UPDATE_TRANSACTION_ID }, | ||
optimist: isAutosave ? false : { type: REVERT, id: POST_UPDATE_TRANSACTION_ID }, | ||
} ); | ||
} ); | ||
}, | ||
REQUEST_AUTOSAVE_EXISTS( action, store ) { | ||
const { autosave } = action; | ||
const { dispatch, getState } = store; | ||
if ( autosave ) { | ||
dispatch( createWarningNotice( | ||
<p> | ||
<span>{ __( 'There is an autosave of this post that is more recent than the version below.' ) }</span> | ||
{ ' ' } | ||
{ <a href={ autosave.edit_link }>{ __( 'View the autosave' ) }</a> } | ||
</p>, | ||
{ | ||
id: AUTOSAVE_POST_NOTICE_ID, | ||
} | ||
) ); | ||
|
||
} | ||
}, | ||
REQUEST_POST_UPDATE_SUCCESS( action, store ) { | ||
const { previousPost, post } = action; | ||
const { previousPost, post, isAutosave } = action; | ||
const { dispatch, getState } = store; | ||
|
||
const publishStatus = [ 'publish', 'private', 'future' ]; | ||
|
@@ -129,8 +190,10 @@ export default { | |
future: __( 'Post scheduled!' ), | ||
}[ post.status ]; | ||
} else { | ||
// Generic fallback notice | ||
noticeMessage = __( 'Post updated!' ); | ||
if ( ! isAutosave ) { | ||
// Generic fallback notice | ||
noticeMessage = __( 'Post updated!' ); | ||
} | ||
} | ||
|
||
if ( noticeMessage ) { | ||
|
@@ -263,25 +326,19 @@ export default { | |
return; | ||
} | ||
|
||
if ( ! isEditedPostNew( state ) && ! isEditedPostDirty( state ) ) { | ||
// Change status from auto-draft to draft, saving the post. | ||
if ( isEditedPostNew( state ) ) { | ||
dispatch( editPost( { status: 'draft' } ) ); | ||
dispatch( savePost() ); | ||
return; | ||
} | ||
|
||
if ( isCurrentPostPublished( state ) ) { | ||
// TODO: Publish autosave. | ||
// - Autosaves are created as revisions for published posts, but | ||
// the necessary REST API behavior does not yet exist | ||
// - May need to check for whether the status of the edited post | ||
// has changed from the saved copy (i.e. published -> pending) | ||
if ( ! isEditedPostDirty( state ) ) { | ||
return; | ||
} | ||
|
||
// Change status from auto-draft to draft | ||
if ( isEditedPostNew( state ) ) { | ||
dispatch( editPost( { status: 'draft' } ) ); | ||
} | ||
|
||
dispatch( savePost() ); | ||
dispatch( isAutosaving( true ) ); | ||
dispatch( savePost( { 'autosave': true } ) ); | ||
}, | ||
SETUP_EDITOR( action ) { | ||
const { post, settings } = action; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess previously, we're stopping the timer if we don't need to autosave anymore, should we do this as well. Maybe something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that makes sense. i guess this could happen if you made a change then undid it before an autosave would have fired? i'm going to review the current core autosave.js again to see what the expected/current behavior is.