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

Show autosave message when autosave exists #4218

Merged
merged 29 commits into from
Jun 4, 2018
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
cc1a856
Rework PR 4218
Apr 13, 2018
c6d5cce
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 18, 2018
3baae76
Merge branch 'master' into feature/heartbeat-autosaving
Apr 18, 2018
3444476
switch to withDispatch approach
Apr 18, 2018
ebe5dd4
fixes for eslint
Apr 18, 2018
045fd0f
remove unneeded braces
Apr 19, 2018
c97b109
REQUEST_AUTOSAVE_NOTICE: bail early if autosaveStatus is falsey
Apr 19, 2018
fb0988e
remove unused autosave reducer
Apr 19, 2018
68f0754
whitespace
Apr 19, 2018
4f2832c
Merge branch 'master' into feature/heartbeat-autosaving
Apr 20, 2018
9194bb7
remove field change check when retrieving autosave newer than post
Apr 20, 2018
5f816d1
cleanup, doc blcoks
Apr 20, 2018
95cb823
exit early if autosave false
Apr 20, 2018
80768f1
Merge branch 'master' into feature/heartbeat-autosaving
Apr 27, 2018
f625239
add missing simicolon
Apr 27, 2018
3e5e744
Merge branch 'master' of github.com:WordPress/gutenberg
May 23, 2018
9e97fed
Merge branch 'master' into feature/heartbeat-autosaving
May 23, 2018
cf9dd40
Merge branch 'master' of github.com:WordPress/gutenberg
May 29, 2018
f1991ad
Merge branch 'master' of github.com:WordPress/gutenberg
May 31, 2018
fb512ce
Merge branch 'master' of github.com:WordPress/gutenberg
May 31, 2018
171c511
Merge branch 'master' of github.com:WordPress/gutenberg
Jun 1, 2018
bdab38b
Merge branch 'master' into feature/heartbeat-autosaving
Jun 1, 2018
6323b25
spacing cleanup
Jun 1, 2018
fcd3d61
fix for eslint
Jun 1, 2018
5ceca78
Remove useless spans
youknowriad Jun 4, 2018
216dd49
Merge remote-tracking branch 'origin/master' into feature/heartbeat-a…
youknowriad Jun 4, 2018
52c75a5
Drop the showAutoSaveMessage action
youknowriad Jun 4, 2018
2054d34
Fix unit tests after removing the span
youknowriad Jun 4, 2018
0c0f8d3
Rename autosaveStatus property just "autosave"
youknowriad Jun 4, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions editor/components/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ class EditorProvider extends Component {
this.props.updateEditorSettings( props.settings );
this.props.setupEditor( props.post );
}

// Display a notice if an autosave exists.
if ( props.settings.autosave ) {
this.props.showAutosaveNotice( props.settings.autosave );
}
}

componentDidUpdate( prevProps ) {
Expand Down Expand Up @@ -106,12 +111,14 @@ export default withDispatch( ( dispatch ) => {
undo,
redo,
createUndoLevel,
showAutosaveNotice,
} = dispatch( 'core/editor' );
return {
setupEditor,
undo,
redo,
createUndoLevel,
showAutosaveNotice,
updateEditorSettings,
};
} )( EditorProvider );
13 changes: 13 additions & 0 deletions editor/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,19 @@ export function autosave() {
return savePost( { autosave: true } );
}

/**
* Returns an action to show the autosave notice.
*
* @param {boolean} autosaveStatus Autosave status and data including a link to the autosave.
* @return {Object} Action object.
*/
export function showAutosaveNotice( autosaveStatus ) {
return {
type: 'REQUEST_AUTOSAVE_NOTICE',
autosaveStatus,
};
}

/**
* Returns an action object used in signalling that undo history should
* restore last popped state.
Expand Down
22 changes: 22 additions & 0 deletions editor/store/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
replaceBlocks,
createSuccessNotice,
createErrorNotice,
createWarningNotice,
removeNotice,
saveSharedBlock,
insertBlock,
Expand Down Expand Up @@ -69,6 +70,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';

Expand Down Expand Up @@ -133,6 +135,7 @@ export default {
} );

dispatch( removeNotice( SAVE_POST_NOTICE_ID ) );
dispatch( removeNotice( AUTOSAVE_POST_NOTICE_ID ) );

request = wp.apiRequest( {
path: `/wp/v2/${ basePath }/${ post.id }`,
Expand Down Expand Up @@ -238,6 +241,25 @@ export default {
__( 'Updating failed' );
dispatch( createErrorNotice( noticeMessage, { id: SAVE_POST_NOTICE_ID } ) );
},
REQUEST_AUTOSAVE_NOTICE( action, store ) {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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 for SETUP_EDITOR†). Or, if the EditorProvider component itself is responsible for deciding to display the notice, then it can dispatch showAutosaveNotice, but in that case I'd see no reason why the logic here shouldn't just occur within the action creator (move to showAutosaveNotice).

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.

SETUP_EDITOR: [
	checkTemplateValidity,
	setupEditorState,
	showAutosaveNotice,
]

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.

const { autosaveStatus } = action;
const { dispatch } = store;
if ( ! autosaveStatus ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this an instance where we're being defensive? My first reaction to this is questioning the cases in which we expect autosaveStatus to be falsey.

Copy link
Member Author

Choose a reason for hiding this comment

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

i am defensive by default in my programming, perhaps to a fault. might be fine to omit this.

return;
}
const noticeMessage = __( 'There is an autosave of this post that is more recent than the version below.' );
dispatch( createWarningNotice(
<p>
Copy link
Member

Choose a reason for hiding this comment

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

Does a notice message need to have <p> or <span> wrappers? Ideally it shouldn't.

<span>{ noticeMessage }</span>
Copy link
Member

Choose a reason for hiding this comment

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

Will anything change if we omit <span> ?

Copy link
Member Author

Choose a reason for hiding this comment

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

good question, i can test again - my guess is this was introduced to match other existing notices.

{ ' ' }
<a href={ autosaveStatus.editLink }>{ __( 'View the autosave' ) }</a>
</p>,
{
id: AUTOSAVE_POST_NOTICE_ID,
spokenMessage: noticeMessage,
}
) );
},
TRASH_POST( action, store ) {
const { dispatch, getState } = store;
const { postId } = action;
Expand Down
36 changes: 36 additions & 0 deletions lib/client-assets.php
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,35 @@ function gutenberg_capture_code_editor_settings( $settings ) {
return false;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Newline before.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

@azaozz azaozz Apr 19, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

@azaozz azaozz Apr 19, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, we could do this with the API instead, good suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

The 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 );

if ( ! $autosave ) {
return false;
}

// Check if the autosave is newer than the current post.
if (
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 - wp-admin/edit-form-advanced.php

Copy link
Member Author

Choose a reason for hiding this comment

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

extracted this

mysql2date( 'U', $autosave->post_modified_gmt, false ) > mysql2date( 'U', $post->post_modified_gmt, false )
Copy link
Member

Choose a reason for hiding this comment

The 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 strcmp on the two date values here?

Copy link
Member Author

@adamsilverstein adamsilverstein Apr 19, 2018

Choose a reason for hiding this comment

The 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.

) {
return $autosave;
}

// If the autosave isn't newer, remove it.
wp_delete_post_revision( $autosave->ID );

return false;
}

/**
* Scripts & Styles.
*
Expand Down Expand Up @@ -1084,6 +1113,13 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
'autosaveInterval' => 10,
);

$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;
}
Expand Down