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

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Jan 1, 2018

Description

Enables heartbeat based autosave and connection monitoring

How Has This Been Tested?

Autosaves

  • Create a local post in the classic editor; create an autosave revision, making some changes, letting autosaave fire, then exiting the screen.
  • Open post in Gutenberg.

Screenshots (jpeg or gifs if applicable):

Opening a post with an existing autosave:

Click 'view the autosave' takes you to the revisions screen where the autosave is selected and you can restore it.

Network disconnected notice (note save and switch to draft disabled):

Types of changes

  • Check for autosave at load
  • show autosave message if present

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation. # #

This was referenced Jan 1, 2018
@adamsilverstein
Copy link
Member Author

I'd like some design input here on two things:

First, the post locking modal.
This pops up when you try to open a post someone is editing, or someone takes over a post you are editing. It looks something like this: https://codex.wordpress.org/Post_Locking

When a user takes over a post you are editing, the modal pops up and you can no longer edit, the only option is to return to the posts list screen.

When you try to open a post someone is editing, the modal offers 'go back', 'preview' and 'take over' - the editor is inactive unless you choose to take over.

Second - the autosave confirmation message
When a heartbeat autosave fires, the server returns a helpful string about the autosave. In the classic editor this is displayed in the footer of the editor:

How do we want to show this save time/status in Gutenberg?

During the save action itself, I am following the classic editor and showing the publish button in a 'saving' state (also disabling them) - this helps prevent save collisions and clues the user in to the autosave happening.

@adamsilverstein
Copy link
Member Author

One more design item: heartbeat/autosave also tracks the connection status, and autosaves the data to localstorage when you are offline. do we want to show an alert with a disconnection message similar to where we show the autosave message?

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Jan 3, 2018
@adamsilverstein
Copy link
Member Author

cc: @jasmussen @karmatosed for design input

@mtias mtias added the [Feature] Document Settings Document settings experience label Jan 3, 2018
@mtias
Copy link
Member

mtias commented Jan 3, 2018

Thanks for working on this. Also looping @gziolo for the collaborative editing work. Ideally when you encounter a locked post you'll see the option to request to collaborate.

@jasmussen
Copy link
Contributor

Sorry for not looking sooner. I'll be back at this tomorrow and I'll have a look. Thanks for working on this!

@jasmussen
Copy link
Contributor

jasmussen commented Jan 4, 2018

Thanks so much for working on this. It works great.

When a user takes over a post you are editing, the modal pops up and you can no longer edit, the only option is to return to the posts list screen.
When you try to open a post someone is editing, the modal offers 'go back', 'preview' and 'take over' - the editor is inactive unless you choose to take over.

Given we'd love to have collaborative editing, like what Matías mentions, it seems like we should either not tackle the post locking as part of this PR, or keep the UI minimal and virtually identical to what's there now.

When a heartbeat autosave fires, the server returns a helpful string about the autosave. In the classic editor this is displayed in the footer of the editor:
How do we want to show this save time/status in Gutenberg?

Do we truly need this when we have this?

screen shot 2018-01-04 at 11 00 06

If yes, then I'd show it in the revisions UI:

screen shot 2018-01-04 at 11 00 16

It could say "2 Revisions Draft saved 8:52:02pm" perhaps?

One more design item: heartbeat/autosave also tracks the connection status, and autosaves the data to localstorage when you are offline. do we want to show an alert with a disconnection message similar to where we show the autosave message?

Yes, solid idea. Can we show a "warning" colored prompt like the publish prompt?

screen shot 2018-01-04 at 11 01 52

@karmatosed
Copy link
Member

I will add a little note that on observing users I have seen page locking to be a huge issue. One particular situation I recall someone saying they had to Slack someone to get page's unlocked.

I would second what @jasmussen has said and not tackle post locking. I actually think collaborative editing holds a solution that could stop a lot of experience issues.

Failing that I also agree keeping what we have now would work. I think an alert to disconnection is good, although maybe a notice color not green as it's a notice?

@youknowriad
Copy link
Contributor

I would second what @jasmussen has said and not tackle post locking. I actually think collaborative editing holds a solution that could stop a lot of experience issues.

Agreed having collaborative editing would be great but right now when I write a post with Gutenberg I can't really share it with someone else while still a draft because if he opens the same post in Gutenberg, I might loose edits I'll be doing because of auto-saving enabled in both sides. I'd personally appreciate post locking if it's simple enough and removable once we have collaborative editing.

@gziolo
Copy link
Member

gziolo commented Jan 4, 2018

Please also keep in mind that Collaborative Editing is going to be disabled by default and working with WebRTC only for 2 collaborators, so I would suggest exploring flows that enable post locking, too.

@gziolo
Copy link
Member

gziolo commented Jan 4, 2018

@adamsilverstein it would be great to integrate at some point hooks into core and retrigger jquery events using actions 😃

@adamsilverstein
Copy link
Member Author

@jasmussen Thanks for the feedback. I like your idea of putting the last autosave status near the revisions indicator, I'll give that a try. You asked if its needed give that we already have a 'Saved' indicator - the short answer is I think this is slightly different - "Saved" means you have actually saved your post and made no further changes. An autosave is a temporary store of your changes since the last save and only used if you exit the editor without saving and then later revisit the editor.

I will remove post locking from the PR for now since there doesn't seem to be consensus on how it should work and I'd like to get autosaving in. If/when we do land post locking, we still need a design for a modal that pops up to show the interruption/locking status. Personally I see post locking as a critical feature, and if collaborative editing is enabled, maybe that can be one of the modal options? eg options are 'collaborate, 'go back', 'preview' and 'take over' when you open a locked post

@adamsilverstein
Copy link
Member Author

I will add a little note that on observing users I have seen page locking to be a huge issue. One particular situation I recall someone saying they had to Slack someone to get page's unlocked.

@karmatosed you mean the current post locking in core, right? I guess the option to take over the post is only for admins or not that users caps anyway. Do you think post locking solves more problems (overwriting others changes) than it causes?

@adamsilverstein
Copy link
Member Author

I might loose edits I'll be doing because of auto-saving enabled in both sides. I'd personally appreciate post locking if it's simple enough and removable once we have collaborative editing.

@youknowriad - I agree post locking is important, and yes without it two users can open the same post and overwrite each others changes by doing a normal save. Clarification however that autosaves will not overwrite other user's autosaves - each user's autosaves are kept separately (one autosave per post per user).

We can't really add post locking until we have a design/implementation for the modal that pops up when you open a locked post - the feature is incomplete without that.

@adamsilverstein
Copy link
Member Author

it would be great to integrate at some point hooks into core and retrigger jquery events using actions

For sure! and that way our actions would have priority so plugins could for example, respond to the data for an autosave by hooking in before our save handler.

@jasmussen
Copy link
Contributor

You asked if its needed give that we already have a 'Saved' indicator - the short answer is I think this is slightly different - "Saved" means you have actually saved your post and made no further changes. An autosave is a temporary store of your changes since the last save and only used if you exit the editor without saving and then later revisit the editor.

Does the user need to understand the difference enough to change the label there? The idea of putting the extra information near the revisions UI is exactly because it feels like an advanced distinction that isn't super meaningful to most people. Not that it sounds like we're disagreeing here, just elaborating.

I will remove post locking from the PR for now since there doesn't seem to be consensus on how it should work and I'd like to get autosaving in. If/when we do land post locking, we still need a design for a modal that pops up to show the interruption/locking status. Personally I see post locking as a critical feature, and if collaborative editing is enabled, maybe that can be one of the modal options? eg options are 'collaborate, 'go back', 'preview' and 'take over' when you open a locked post

When I tried this PR with two different users (one in an incognito mode), I didn't see any post-locking UI. But given what @gziolo suggested here ...

Please also keep in mind that Collaborative Editing is going to be disabled by default and working with WebRTC only for 2 collaborators, so I would suggest exploring flows that enable post locking, too.

... maybe we shouldn't remove the post locking UI after all.

I mean, it doesn't have to be in this PR — if you like I can do a mockup first, or if you like you can implement some barebones feature and I can CSS it up for you?

@adamsilverstein
Copy link
Member Author

Does the user need to understand the difference enough to change the label there?

Possibly not - good point. In the current (or previous) PR I do use the same string/spot to indicate that an autosave is happening 'Autosaving'... 'Saved'... maybe that is all we need.

When I tried this PR with two different users (one in an incognito mode), I didn't see any post-locking UI.

The only thing in place so far is the locking itself, you should see this show up as a lock icon/status indicator in the post list screen. if you try opening the post in the classic editor you should see the locking warning modal. If not, something may not be working.

... maybe we shouldn't remove the post locking UI after all.

I mean, it doesn't have to be in this PR — if you like I can do a mockup first, or if you like you can implement some barebones feature and I can CSS it up for you?

Yes, a mock up would be great, are there any other examples in gutenberg of a modal display? Yes, I'd like to break off post locking to a separate PR and change this one to focus on autosave only, which is closer to being ready.

@adamsilverstein adamsilverstein changed the title Enable Heartbeat API autosaving and post locking Enable Heartbeat API autosaving Jan 5, 2018
* @returns {string} A concatenated string with title, content and excerpt.
*/
const getCompareString = function( state ) {
return ( getEditedPostTitle( state ) || '' ) + '::' + ( getEditedPostContent( state ) || '' ) + '::' + ( getEditedPostExcerpt( state ) || '' );
Copy link
Member

Choose a reason for hiding this comment

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

I might suggest a combination of Array#join and Lodash's _.compact:

return compact( [
	getEditedPostTitle( state ),
	getEditedPostContent( state ),
	getEditedPostExcerpt( state ),
] ).join( '::' );

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, thats much cleaner and easier to read, I'll update it. This function was copied from autosave.js and changed to use the editor state rather than reading from the dom.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 838f78f

Copy link
Member

Choose a reason for hiding this comment

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

This function was copied from autosave.js and changed to use the editor state rather than reading from the dom.

Does it need to mimic the behavior of autosave.js to respect previous autosaves? My suggested implementation is slightly different in that redundant joiners would be dropped, so e.g if content was empty and the others not:

// Before:
"title::::excerpt"

// After:
"title::excerpt"

@@ -52,6 +52,9 @@ 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 ) );
if ( props.autosave ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there ever a case where props.recovery && props.autosave ? Do we need to nest this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not, un-nested in 1ee973e

editor/index.js Outdated
const target = document.getElementById( id );
const reboot = recreateEditorInstance.bind( null, target, settings );
setupHeartbeat();
Copy link
Member

Choose a reason for hiding this comment

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

Would this be able to handle multiple editors on the same page? To that point, does our revised approach of a singleton editor store support this at all? (cc @youknowriad)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly heartbeat should only be initialized once, but yes - autosaving should work with multiple editors - its their listeners that 'trigger' the autosave by attaching and responding to data attached to the heartbeats - each editor would handle its own local data/state...

On merging to core, we may want to change how this initialized and consider adding the autosave customizations into autosave.js directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess multiple editor do not work, we should rename this function initializeEditor or something which suggested you can't call it twice for multiple instances. (not related to this PR though)

@adamsilverstein
Copy link
Member Author

Here is what the autosave alert looks like when you open a post with an existing autosave:

Click 'view the autosave' takes you to the revisions screen where the autosave is selected and you can restore it.

@adamsilverstein
Copy link
Member Author

@jasmussen noticed the 'Saved' area for published posts and now shows 'Switch to Draft'. That seems way to prominent for such a rarely used action, should that be a bit harder to reach? Also, not sure where to show the autosaving status for published posts. For now, the only indication is the publish button gets the active blue stripes (same as 'isSaving') for around 5 seconds when the autosave data is attached to a heartbeat - this also disables the update action and helps prevent save collisions.

@adamsilverstein
Copy link
Member Author

I started working on local storage autosaving is a separate branch(https://github.com/WordPress/gutenberg/compare/feature/local-saving-for-heartbeat-autosaves?expand=1) - For now, I'd like to get this PR merged so we have autsaving actually working in Gutenberg.

@adamsilverstein
Copy link
Member Author

Network disconnected notice (note save disabled):

@jasmussen
Copy link
Contributor

noticed the 'Saved' area for published posts and now shows 'Switch to Draft'. That seems way to prominent for such a rarely used action, should that be a bit harder to reach?

I believe @mtias has some background on the value of this action, and perhaps @karmatosed can add this to the list of things to test. But the argument for putting this here is that you only see it once the post has been published. At this point, the value of showing "Saved" prominently in the editor bar (especially if we can show recent local storage saves in the Revisions UI in the sidebar) feels reduced, as one would think edits to published posts are rarer than edits to unpublished posts, and because "Switch to Draft" seems to imply that the content is saved.

Also, not sure where to show the autosaving status for published posts. For now, the only indication is the publish button gets the active blue stripes (same as 'isSaving') for around 5 seconds when the autosave data is attached to a heartbeat - this also disables the update action and helps prevent save collisions.

Given the hypothesis that updating published posts happens slightly more rarely than non-published posts, is it okay if we relegate this info to the revisions UI in the sidebar?

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.

Notices as a whole are in a bit of a messy state right now, and I'd agree we don't need to tackle all of the issues here, as they're not really specific to what you're proposing. Some problems I identify are:

  • Patterns for attaching notices as effects of other behaviors (some related discussion at Data Module: Add support for action creators as generators #6999)
  • Notice handling moved outside editor to a generalized component with its own state (similar to ./viewport)
  • Consistent UX for notices with actions
    • You shouldn't need to generate the p, nor the a, this should be instead expressed as something like:
createWarningNotice( noticeMessage, {
	action: {
		text: __( 'View the autosave' ),
		url: autosaveStatus.editLink,
	}
} );

@@ -202,6 +205,24 @@ 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.

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.

REQUEST_AUTOSAVE_NOTICE( action, store ) {
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.

const noticeMessage = __( 'There is an autosave of this post that is more recent than the version below.' );
dispatch( createWarningNotice(
<p>
<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.

@danielbachhuber
Copy link
Member

Just tested this locally. Looks like it's getting there :)

A couple of notes:

  1. Autosaved published posts have (no title) as the autosave title:

image

I'd expect an actual title in that field.

  1. For the life of me, I couldn't get an autosave created for a draft post. Is this expected behavior? It seems desirable to me, but I want to make sure it's deliberate.

More specifically, when I edit a draft post, the autosave behavior saves the post itself, and restores me to that latest version:

autosavepost

@danielbachhuber
Copy link
Member

Autosaved published posts have (no title) as the autosave title:

Ah, this is reported in #7078

@adamsilverstein
Copy link
Member Author

For the life of me, I couldn't get an autosave created for a draft post.

I think this is the expected behavior, let me double check and also review current behavior. For drafts, I think the difference between an autosave and clicking the 'save' button is that clicking the save button creates a revision. an autosave does not.

@adamsilverstein
Copy link
Member Author

Autosaved published posts have (no title) as the autosave title:

This should be fixed in #7092

@adamsilverstein
Copy link
Member Author

For the life of me, I couldn't get an autosave created for a draft post.

@danielbachhuber - I verified this is the expected behavior. This is exactly the same in the classic editor:

  • draft posts 'autosave' to the draft.
  • clicking the 'save draft' button creates a new revision
  • no special autosave records are created for draft posts (autosaves have a post_name of ID-autosave-VERSION (where version is currently 1)

@danielbachhuber danielbachhuber added this to the 3.0 milestone Jun 4, 2018
@aduth
Copy link
Member

aduth commented Jun 4, 2018

@danielbachhuber @azaozz Do you have remaining feedback here?

@danielbachhuber
Copy link
Member

@aduth I think we should land as-is and clean up the PHP in the next release.

@youknowriad youknowriad force-pushed the feature/heartbeat-autosaving branch from ed8cede to 2054d34 Compare June 4, 2018 14:48
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.

LGTM 👍

@youknowriad youknowriad merged commit 7141ce6 into master Jun 4, 2018
@youknowriad youknowriad deleted the feature/heartbeat-autosaving branch June 4, 2018 15:12
@youknowriad
Copy link
Contributor

Thanks all and great work @adamsilverstein

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Document Settings Document settings experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.