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

Autosave drafts after delay #1945

Merged
merged 3 commits into from
Jul 26, 2017
Merged

Autosave drafts after delay #1945

merged 3 commits into from
Jul 26, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 19, 2017

Partial resolution for #1773

This pull request seeks to implement an incomplete autosave behavior, currently applied to drafts only. Published posts lack API endpoint requirements for creating autosaves (see #1773 (comment)) and will need to be implemented separately. Further improvements may be needed for handling autosave for private or scheduled posts, currently ignored as of these changes.

Implementation notes:

Autosave occurs on a 10 second debounce. The current post editor hooks to heartbeat lock refresh to trigger an autosave. More consideration may be needed for autosave frequency and use of browser storage for locally saved copies of the post. Discussion may be better reserved for #1773, though this pull request seeks to start exploring a minimal solution.

Testing instructions:

Verify that autosave occurs for a draft 10 seconds after a change:

  1. Navigate to Gutenberg > New Post
  2. Enter a title
  3. Wait 10 seconds
  4. Note that the draft saves

@westonruter
Copy link
Member

Autosave is also closely related to the need to previewing changes on the frontend: #1673.

@mtias mtias added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Jul 24, 2017
@aduth aduth requested a review from youknowriad July 24, 2017 14:11
REPLACE_BLOCKS: () => queueAutosave(),
REMOVE_BLOCKS: () => queueAutosave(),
EDIT_POST: () => queueAutosave(),
MARK_DIRTY: () => queueAutosave(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need all these effects? I suspect dirtiness? Can't we use an internal somewhere instead?

Copy link
Member Author

@aduth aduth Jul 24, 2017

Choose a reason for hiding this comment

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

Why do we need all these effects? I suspect dirtiness? Can't we use an internal somewhere instead?

Yeah, all the potentially dirty-ing effects, see also: reducer for editor.dirty.

As remarked in #1967, I'd like to look to reimplementing dirtiness as a diff against the original saved post, in which case we could reimplement this as a subscribe on the store (i.e. if ( currentDirty !== lastDirty ) queueAutosave()).

* @param {Object} state Global application state
* @return {Boolearn} Whether the post has been bublished
*/
export function isEditedPostPublished( state ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble understanding if this is really necessary because the status is changed to 'publish', 'private' only when saving. so no real difference with isCurrentPostPublished. Btw, good renaming.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I recall correctly, the issue is that if the user chooses a date in the past for their draft post, the edited representation of the post becomes "published", but we can't necessarily use the published autosave flow because the saved post is still a draft.

... I think there's still something amiss here though, because in the AUTOSAVE effect above I'm still testing against the edited version of the post, not the "current" (saved) copy.

}

if ( isEditedPostPublished( state ) ) {
// TODO: Publish autosave
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the desired behaviour here?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the desired behaviour here?

Published posts have a distinct auto-save behavior which is not currently possible to implement due to lacking API support: #1773 (comment)

@mtias mtias self-requested a review July 25, 2017 18:16
Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

Works well for me.

@aduth
Copy link
Member Author

aduth commented Jul 25, 2017

I'm trying to sort through what should be the autosave behavior, particularly around transitioning status. The obvious ones are draft staying draft, and published staying published, but it becomes less clear when the post status is modified in the same session:

Original Status Status Pending Save Autosave Behavior
Draft Draft Save draft
Draft Scheduled Save draft
Published Published Autosave (Not Yet Implemented)
Draft Privately Published ??? (Nothing?)
Published Draft ??? (Nothing?)

@youknowriad
Copy link
Contributor

  • For the 4th, I'd say auto-save as Draft but keep the "status pending save" in the edits.
  • The 5th is more confusing but I don't think we have this possibility in the UI (currently).

Do you know how does Calypso resolve this?

@mtias
Copy link
Member

mtias commented Jul 26, 2017

In general, the changes of status in Calypso performs the action. That's why we have "revert to draft" as a button, to try to keep it binary.

@jasmussen we should look again at the status area from the point of view of published/draft. Related: #1726

"edited post" in context of selectors should refer to current post + edited values. Current usage tests against saved post only. Needed to distinguish for "isEditedPostPublished" testing current post with edited values applied.
@aduth aduth force-pushed the update/1773-autosave-draft branch from 1a47c9d to 56d6dfa Compare July 26, 2017 16:03
@aduth
Copy link
Member Author

aduth commented Jul 26, 2017

Rebased and force-pushed with some new tests. Also updated the condition for handling published autosaves to checking the current (last saved) post in deciding whether to abort. When auto-saving a post which had originally been published, we may need some additional logic, but this can be implemented separately. In the meantime I've included some thoughts as inline comments.

I'd have liked to have been more strict with how autosave behaves depending on the actual data of the edited post. For example, while not possible by user flows, if a draft is applied an unsaved edit of status: 'publish', an auto-save could occur which publishes the post automatically. Again, this won't occur with any current user flows, but seems like it'd be nice to capture at the autosave behavior nonetheless.

@aduth aduth merged commit d9752a5 into master Jul 26, 2017
@aduth aduth deleted the update/1773-autosave-draft branch July 26, 2017 16:55
Tug pushed a commit that referenced this pull request Feb 28, 2020
…-android

[Android] Add logging to Page Template Picker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants