-
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
Show autosave message when autosave exists #4218
Changes from 5 commits
cc1a856
c6d5cce
3baae76
3444476
ebe5dd4
045fd0f
c97b109
fb0988e
68f0754
4f2832c
9194bb7
5f816d1
95cb823
80768f1
f625239
3e5e744
9e97fed
cf9dd40
f1991ad
fb512ce
171c511
bdab38b
6323b25
fcd3d61
5ceca78
216dd49
52c75a5
2054d34
0c0f8d3
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 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -34,6 +34,7 @@ import { | |||||||||||||||||
replaceBlocks, | ||||||||||||||||||
createSuccessNotice, | ||||||||||||||||||
createErrorNotice, | ||||||||||||||||||
createWarningNotice, | ||||||||||||||||||
removeNotice, | ||||||||||||||||||
savePost, | ||||||||||||||||||
saveSharedBlock, | ||||||||||||||||||
|
@@ -71,6 +72,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 SHARED_BLOCK_NOTICE_ID = 'SHARED_BLOCK_NOTICE_ID'; | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -109,6 +111,7 @@ export default { | |||||||||||||||||
optimist: { type: BEGIN, id: POST_UPDATE_TRANSACTION_ID }, | ||||||||||||||||||
} ); | ||||||||||||||||||
dispatch( removeNotice( SAVE_POST_NOTICE_ID ) ); | ||||||||||||||||||
dispatch( removeNotice( AUTOSAVE_POST_NOTICE_ID ) ); | ||||||||||||||||||
const basePath = wp.api.getPostTypeRoute( getCurrentPostType( state ) ); | ||||||||||||||||||
wp.apiRequest( { path: `/wp/v2/${ basePath }/${ post.id }`, method: 'PUT', data: toSend } ).then( | ||||||||||||||||||
( newPost ) => { | ||||||||||||||||||
|
@@ -202,6 +205,24 @@ export default { | |||||||||||||||||
__( 'Updating failed' ); | ||||||||||||||||||
dispatch( createErrorNotice( noticeMessage, { id: SAVE_POST_NOTICE_ID } ) ); | ||||||||||||||||||
}, | ||||||||||||||||||
REQUEST_AUTOSAVE_NOTICE( action, store ) { | ||||||||||||||||||
const { autosaveStatus } = action; | ||||||||||||||||||
const { dispatch } = store; | ||||||||||||||||||
const noticeMessage = __( 'There is an autosave of this post that is more recent than the version below.' ); | ||||||||||||||||||
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. Minor: 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. Fixed |
||||||||||||||||||
if ( autosaveStatus ) { | ||||||||||||||||||
dispatch( createWarningNotice( | ||||||||||||||||||
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. Minor: Effects can return action objects, tying into the idea of side effects 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. So, to clarify: returning the action here instead of dispatching it will have the same result? |
||||||||||||||||||
<p> | ||||||||||||||||||
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. Do we need 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'm following the exact patter from above in the same file: gutenberg/editor/store/effects.js Lines 172 to 179 in ebe5dd4
Can we tackle refactoring the notices API in a separate PR? |
||||||||||||||||||
<span>{ noticeMessage }</span> | ||||||||||||||||||
{ ' ' } | ||||||||||||||||||
{ <a href={ autosaveStatus.editLink }>{ __( 'View the autosave' ) }</a> } | ||||||||||||||||||
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. Curly braces are unnecessary. 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 |
||||||||||||||||||
</p>, | ||||||||||||||||||
{ | ||||||||||||||||||
id: AUTOSAVE_POST_NOTICE_ID, | ||||||||||||||||||
spokenMessage: noticeMessage, | ||||||||||||||||||
} | ||||||||||||||||||
) ); | ||||||||||||||||||
} | ||||||||||||||||||
}, | ||||||||||||||||||
TRASH_POST( action, store ) { | ||||||||||||||||||
const { dispatch, getState } = store; | ||||||||||||||||||
const { postId } = action; | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -901,6 +901,24 @@ export function notices( state = [], action ) { | |
return state; | ||
} | ||
|
||
/** | ||
* Reducer returning the currently autosave message. | ||
* | ||
* @param {Object} state Current state. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export function autosave( state = { message: '' }, action ) { | ||
switch ( action.type ) { | ||
case 'UPDATE_AUTOSAVE_STATUS_MESSAGE': | ||
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. Where is this being dispatched? 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. oops, this was a leftover; removed. |
||
return { | ||
...state, | ||
message: action.message, | ||
}; | ||
} | ||
} | ||
|
||
export const sharedBlocks = combineReducers( { | ||
data( state = {}, action ) { | ||
switch ( action.type ) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -799,6 +799,41 @@ function gutenberg_capture_code_editor_settings( $settings ) { | |
$gutenberg_captured_code_editor_settings = $settings; | ||
return false; | ||
} | ||
/** | ||
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. Nitpick: Newline before. 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. fixed |
||
* Retrieve a stored autosave that is newer than the post save. | ||
* | ||
* Deletes autosaves that are older than the post save. | ||
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 exactly true. Deletes one identical autosave only when it is 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. Will adjust docblock |
||
* | ||
* @param WP_Post $post Post object. | ||
* @return WP_Post|boolean The post autosave. False if none found. | ||
*/ | ||
function get_autosave_newer_than_post_save( $post ) { | ||
// Add autosave data if it is newer and changed. | ||
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. Shouldn't this be done through the API. Why do we need the two "get" endpoints for autosaves? They are really pointless if we can't get the latest autosave revision. 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. Yea, we could do this with the API instead, 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. 👍 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. (once the API includes the ability to retrieve autosaves 😄 ) |
||
$autosave = wp_get_post_autosave( $post->ID ); | ||
$show_autosave = false; | ||
|
||
// Check if we have an autosave that is newer than the post and different from the current post. | ||
if ( | ||
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. This logic is slightly more complex than I'm comfortable inlining in the script enqueue function. Could be externalized, ideally with some more comments clarifying what's being done in looping over revision fields. 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, I can change this. Please note this code comes verbatim from core - 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. extracted this |
||
$autosave && | ||
mysql2date( 'U', $autosave->post_modified_gmt, false ) > mysql2date( 'U', $post->post_modified_gmt, false ) | ||
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 if this logic is copied from elsewhere, but would it be enough to do a 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. this is existing logic from core and I'd prefer to leave unchanged to avoid the risk on unintended side effects. |
||
) { | ||
// Iterate thru revisioned fields checking for any changes. | ||
foreach ( _wp_post_revision_fields( $post ) as $autosave_field => $_autosave_field ) { | ||
if ( normalize_whitespace( $autosave->$autosave_field ) != normalize_whitespace( $post->$autosave_field ) ) { | ||
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. This is not needed any more. I know it exists in edit-form-advanced.php but it is here to catch identical autosaves when editing super old posts (from before 2.7 I think). I'd be +1 to skip it here. 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. do you mean the entire check for changed revisioned fields or just the normalize_whitespace part? 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. Uh, sorry for not being clear. Yes, the entire check. It was intended to fix existing posts that used to have identical autosave revisions at some point, very long tie ago :) 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, will clean this up. |
||
$show_autosave = true; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
// If this autosave isn't newer and different from the current post, remove. | ||
if ( $autosave && ! $show_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. This logic is different than the one in edit-form-advanced.php. Why would you want to delete older autosaves here? 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. Trying to match this: https://core.trac.wordpress.org/browser/tags/4.9.5/src/wp-admin/edit-form-advanced.php#L217, possibly this is different. essentially when the editor loads if the autosave isn't going to be shown (because its older) than the published post) delete it. 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. The whole bit of code from there:
Note how the As mentioned above, this is an old bit of code that is not needed any more. Can keep it if needed but the logic has to be exactly the same as in edit-form-advanced.php. 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, thanks for clarifying. I'll bring the existing logic over exactly. I think it is worth deleting stale revisions if only for cleanup purposes. |
||
wp_delete_post_revision( $autosave->ID ); | ||
return false; | ||
} | ||
|
||
return $autosave; | ||
} | ||
|
||
/** | ||
* Scripts & Styles. | ||
|
@@ -951,6 +986,13 @@ function gutenberg_editor_scripts_and_styles( $hook ) { | |
'bodyPlaceholder' => apply_filters( 'write_your_story', __( 'Write your story', 'gutenberg' ), $post ), | ||
); | ||
|
||
$post_autosave = get_autosave_newer_than_post_save( $post ); | ||
if ( $post_autosave ) { | ||
$editor_settings['autosave'] = array( | ||
'editLink' => add_query_arg( 'gutenberg', true, get_edit_post_link( $post_autosave->ID ) ), | ||
); | ||
} | ||
|
||
if ( ! empty( $color_palette ) ) { | ||
$editor_settings['colors'] = $color_palette; | ||
} | ||
|
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.
It's not clear to me that this needs to be a side effect, rather than just a dispatch to
createWarningNotice
directly. The only thing that seems to make it fit as a side effect is that we want a reference to its ID. But I'm also wondering if we should try to adopt a more consistent / scalable approach for the types of notices we expect to be dismissed on a save operation.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'm trying to follow the existing pattern here, i don't really understand what is best here so appreciate any further guidance if doing it this way isn't right.
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.
A more general approach does seem useful - I can imagine an entire class of messages that would display but not reappear once you saved the post. Can we get this PR merged, then open an issue and tackle the generalization in a separate PR?
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.
Elsewhere in this file, we create notices as incidental behaviors of something else: A successful or saving fail may have the effect of displaying a notice, but it's not engrained to the behavior of the save itself. For autosaves, I think we _could+ have it be an effect of the initialization (i.e.
SETUP_EDITOR
) to show the autosave notice (move this into a new effect handler forSETUP_EDITOR
†). Or, if theEditorProvider
component itself is responsible for deciding to display the notice, then it can dispatchshowAutosaveNotice
, but in that case I'd see no reason why the logic here shouldn't just occur within the action creator (move toshowAutosaveNotice
).I don't feel strongly either way between these alternatives, but as implemented it seems awkward because
REQUEST_AUTOSAVE_NOTICE
serves no purpose on its own aside from (and is thus inseparable from) the effect logic taking place here.† Ideally implemented as its own effect, where the value of a key in this object can be an array, e.g.
We're not applying this pattern very well in
effects
in general, though this is more what I'd like to see than adding to an already large and convoluted effect handler.