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

Save edit form as draft using localStorage #4252

Open
wants to merge 71 commits into
base: main
Choose a base branch
from
Open

Conversation

tiberiuichim
Copy link
Contributor

@tiberiuichim tiberiuichim commented Jan 11, 2023

Fixed #4168

@netlify
Copy link

netlify bot commented Jan 11, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 4b972da
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65e367ef7d0cec000813af59
😎 Deploy Preview https://deploy-preview-4252--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tiberiuichim
Copy link
Contributor Author

@sneridagh Take a look at the UX, let me know how it feels to you.

How to test:

  • edit a content item
  • make some changes
  • click the X cancel toolbar button
  • edit again
  • you'll see a "Autosave found, load it?" confirm dialog. If yes, it loads the saved value, otherwise the form is left intact

@tiberiuichim
Copy link
Contributor Author

Some things to discuss:

  • do we want this for the Add item screen? It may be useful in have some cases, but I have concerns about dates saved in the form and disregarded by the editors
  • do we want to configure the autosave interval (at 300ms right now)
  • do we want to make it disable-able for some scenarios? Content types, user roles, etc

@stevepiercy
Copy link
Collaborator

  • you'll see a "Autosave found, load it?" confirm dialog. If yes, it loads the saved value, otherwise the form is left intact

It would be nice to see a Cypress test with a video, else a video capture of your own screen. That would help me comment on the above item.

do we want this for the Add item screen? It may be useful in have some cases, but I have concerns about dates saved in the form and disregarded by the editors

I would weigh this against the current situation, where not having autosave would be much worse. Closing a browser window by accident happens all too often. In the scenario you describe, the worst thing that would happen is that dates do not get updated, but they could still be corrected by editors.

do we want to configure the autosave interval (at 300ms right now)

I would want to make it configurable with a sensible default value. I don't have an opinion of "sensible" now, so use your best judgment.

Also there are two parameters to consider: absolute interval (every N ms) and idle interval (duration of no keyboard or mouse activity in the active window). The latter might be too tricky to achieve, but it would reduce bandwidth (assuming autosave sends an HTTP request) and writes to persistent storage.

do we want to make it disable-able for some scenarios? Content types, user roles, etc

I cannot think of any scenario where it would be useful to disable autosave. Can you think of a common use case?

@tiberiuichim
Copy link
Contributor Author

@stevepiercy Thanks for the input! Sorry for not including any other visual clues, I didn't want to spend too much effort if the approach is not correct.

Also there are two parameters to consider: absolute interval (every N ms) and idle interval (duration of no keyboard or mouse activity in the active window). The latter might be too tricky to achieve, but it would reduce bandwidth (assuming autosave sends an HTTP request) and writes to persistent storage.

I used a debounce technique to handle this case. Whenever the user changes the form state, we schedule a future "commit" (while canceling any active scheduling). If there's no keyboard input, there's no state change. We don't use an interval-based save.

@tiberiuichim
Copy link
Contributor Author

@stevepiercy Ok, so about the add screen. Do we want the saved values to be restricted to the location, or can we say "I wanted to add a Document at location /x, but I gave up, moved to location /y, I want the previous values entered to be restored".

@stevepiercy
Copy link
Collaborator

I think I need a little more background before I can comment on the "restore autosaved item" feature.

Let's say a content type gets autosaved. Would it show up in that location's folder, such as this screenshot?

Screen Shot 2023-01-11 at 11 32 39 PM

In that screenshot, I created a News content type at / with the title of "Test Title". Would an autosaved item appear here, and if so, what would it look like?

Sorry for coming late to the party, if you have already discussed this piece.

If I understand what you are proposing, I prefer to avoid unexpected dialogs in the UI. For example, if I navigate to a location, then click Add, I want to add something and not be asked if I want to recover something else. If I want to resume work on something, I would navigate to the directory listing. There I can manage it as usual.

I can see where it would be helpful for an editor to be prompted to recover what was autosaved, but it would also annoy the heck out of people who just want to add a new item.

@tiberiuichim
Copy link
Contributor Author

@stevepiercy no no, this feature doesn't interact with the server, it only saves entered fields in the browser of the editor.

vokoscreenNG-2023-01-12_10-13-31.mp4

@stevepiercy
Copy link
Collaborator

Ah, OK, I see now. I would love to see autosave implemented server-side in another PR.

For this PR, is the alert a React component with potential UI options (two buttons, not just one), and not a JavaScript alert with limited UI options? The source code changes appear to be the former.

Assuming that is the case, then here is what I would suggest.

For autosaved content, save one maximum of each content type per location. Assume the following:

  • /a has 1 News and 1 Page
  • /b has 1 Event
  1. The editor navigates to /a and clicks Add > News, they would see the alert, "You have a previously autosaved News item. [Edit] [Delete]" (two buttons).
  2. If instead they click Add > Event, it would not display an alert and go straight into editing mode.
  3. The editor navigates to /b, and clicks Add > Event, they would see, "You have a previously autosaved Event item. [Edit] [Delete]".

I think more than one content type per location would be unmanageable and impractical. I hope I understand what the intent is now. Please let me know.

@tisto
Copy link
Member

tisto commented Jan 12, 2023

@tiberiuichim w00t! Awesome that you started to work on this!

Am I assuming correctly that we autosave a version even when the user clicks on the "X" button just for the purpose of showing that it works in a PoC? I wouldn't expect the system to save a version that I explicitly discarded as a user.

@tiberiuichim
Copy link
Contributor Author

@tisto Something like that. I've actually tested with the "discard my saved form when I click cancel", it's this commented line:

// this.props.onCancelDraft();

The problem is that the Users don't have a way of going back to the View page of an item, except either by the Cancel or by Save buttons. So it's up to us to decide how we want this whole thing to work, what its meaning should be in the overall workflow.

As a side discussion, I have another idea for UX, for the Confirm() dialog that restores the saved state. Rather then letting the user decide what to do about the saved state, what if we just load that state and show a Toast message saying something like "Found a previous unsaved version, it is now loaded. You can undo and load the saved version".

@ksuess
Copy link
Member

ksuess commented Jan 12, 2023

  • you'll see a "Autosave found, load it?"

This needs a check if the page has been edited since, doesn't it?

@tiberiuichim
Copy link
Contributor Author

@ksuess Indeed, it would be a good idea to check last modification times, in case another user edited a document.

@davisagli
Copy link
Member

@tiberiuichim Thanks for starting work on this!

As a side discussion, I have another idea for UX, for the Confirm() dialog that restores the saved state. Rather then letting the user decide what to do about the saved state, what if we just load that state and show a Toast message saying something like "Found a previous unsaved version, it is now loaded. You can undo and load the saved version".

I like this idea.

Do we need any special considerations for how it interacts with edit locking?

@tiberiuichim
Copy link
Contributor Author

@davisagli no, it all happens client side based on saved data in the localstorage.

The current internal behavior is like this:

  • user opens edit form
  • on any change, we schedule a debounced localStorage.save()
  • when there's no activity from the user, the current form content is comitted to localstorage
  • if the user clicks Save, the localstorage is removed. If they click Cancel, it is not removed (to be decided on this behavior).
  • if the user reopens the Edit page, the localstorage is compared with the current formData. If they're different, the user is prompted what to do: either load or dump the previous saved version.

As the key for the localstorage, we use this algorithm:

const getFormId = (props) => {
  const { type, pathname, isEditForm } = props;

  const id = isEditForm
    ? ['form', type, pathname].join('-')
    : type
    ? ['form', 'add', type].join('-')
    : ['form', pathname].join('-');

  return id;
};

@stevepiercy
Copy link
Collaborator

if the user clicks Save, the localstorage is removed. If they click Cancel, it is not removed (to be decided on this behavior).

Third option: the user quits the browser, closes the tab, or otherwise navigates away (clicks a browser bookmark). What happens? In this scenario, an alert of "Are you sure you want to destroy your work?" would be super helpful to prevent accidents. Let's call it "diaper mode".

@tisto
Copy link
Member

tisto commented Sep 18, 2023

One thing that I would like to discuss is the wording. When I hear "autosave" I think about the autosave functionality in Google Docs, which is not what we implemented here. What about calling this "content recovery"? Any other ideas? @rexalex @stevepiercy @tiberiuichim @sneridagh @davisagli @ksuess

@stevepiercy
Copy link
Collaborator

Recovery is not the right term because the content was never lost. The autosaved content is a version of stored content.

@sneridagh
Copy link
Member

@tisto @stevepiercy How about "local autosave" to clarify it's not being persisted in server?

@stevepiercy
Copy link
Collaborator

I don't see a problem with plain old "autosave". Most end users won't know the difference of where content is autosaved. If you qualify it with "local" or "remote", that could increase confusion.

@tisto
Copy link
Member

tisto commented Sep 19, 2023

@stevepiercy say you are working on something in an application (like LibreOffice) and your OS crashes. The application will ask you next time you open it if you want to "recover the document", see LibreOffice:

libreoffice-document-recovery

I never saw a modal asking if I wanted to access my "autosaved document". I am not a native speaker though. Our use case and what it solves is similar to the document recovery (the browser crashes or is closed, we ask the user if she wants it back).

@davisagli
Copy link
Member

@stevepiercy I think @tisto has a point. When I hear "autosave", I assume that means that I never have to click a save button because it always happens automatically. That's not what this feature is.

@stevepiercy
Copy link
Collaborator

@davisagli I thought this feature always autosaves content locally after the user stops interacting with the content after a defined duration, right?

I think the question is what do you call the process of loading that autosaved content? Did the user close the browser window or navigate away from the page without saving the content first? Did the app or browser crash? Was the user's intent deliberate, accidental, or completely outside their control?

"Recover" is appropriate in the LIbreOffice context. It attempts to recover lost data from a fatal disaster outside the user's intent. Recovery does not happen if the user deliberately closes the file.

Perhaps the term "restore" would be a more appropriate verb than "load" and "recover"? Google Drive uses "restore" not "recover" in its version history feature. Its autosave feature is similar to my understanding of this feature in Volto. For example, in a Google Doc, if I type something and immediately close the document before it saves to the remote drive, then the changes are not autosaved. If the changes get autosaved, either because there was sufficient time or I have Edit Offline turned on, then I can restore an autosaved version, albeit only the last autosaved version.

From the Merriam-Webster site:

https://www.merriam-webster.com/dictionary/restore

Screenshot 2023-09-19 at 12 23 04 AM

https://www.merriam-webster.com/dictionary/recover

Screenshot 2023-09-19 at 12 22 40 AM

@sneridagh
Copy link
Member

@tisto @stevepiercy I'd like to move forward on this one, and release it right away. Right now it's blocking 17, and we are approaching the PC quickly.

Could we get to an agreement? Tomorrow we have the Volto Team Meeting.

Thanks!

@stevepiercy
Copy link
Collaborator

@sneridagh I'm awaiting a response from @davisagli or @tisto to my most recent comment. I might be wrong about what this feature actually does.

@sneridagh
Copy link
Member

Volto Team Meeting (2023-09-26) decisions:

  • We settled in "Restore" as the term to use in all literals (as opposite of "autosave").
  • We had the feeling that it needs more polishment, especially in the UX interactions, so we will not rush and wait for the next alpha phase and slate it for 18.
  • If it seems sensible, add a diff view feature in case of conflict with the current "saved in server" version, if any (at the moment of the meeting we didn't know if that could even happen at some point).
  • Try to determine if it would be also an option as a complement to this feature to introduce a "form unload" protection mechanism.
  • It seems sensible to put this feature under a feature flag

@djay
Copy link
Member

djay commented Sep 27, 2023

As a side discussion, I have another idea for UX, for the Confirm() dialog that restores the saved state. Rather then letting the user decide what to do about the saved state, what if we just load that state and show a Toast message saying something like "Found a previous unsaved version, it is now loaded. You can undo and load the saved version".

I think @tiberiuichim idea got lost somehow.

This feature should not be a recovery from saved version choice like libreoffice (I hate this UI). You are giving the user a choice while giving them no information on how to make the choice and it often comes up when you are trying to do something else.

Instead this should be considered the "unsaved work" feature* and it could work like this:

  • if you edit then close the browser and then later reedit
    • you get a toast to say "you are editing unsaved content x days old. click cancel to revert to the saved version"
  • if you edit and close the browser and someone else tried to edit
    • they can't. it's locked
  • if you edit and close the browser and someone overrides the lock and edits and then you go back in and edit
    • you get a toast to say you lost your changes. bad luck.
    • this is niche case anyway
    • later this can be improved with an option to save as a working copy maybe.
  • if you edit and then cancel
    • I suspect the best thing to do is just abandon the local changes. The user is explicitly cancelling. Putting up a dialog each time they cancel to explain that they can come back to where they left off might be a pain if what they are trying to do is revert to the saved copy. Instead click off the page could put up a toast that their changes were saved in their browser and they will learn. but I'm 50/50 on this. a back button + cancel button is maybe the best option.

All of this means we can avoid having diffs of the local version until later and the changes to this PR to include it earlier are very minimal both technically and to the current user flows @davisagli @tisto @sneridagh ?

But later it would be nice to move the history view into editmode and then you can diff your unsaved changes against the saved version or any other. No special diff mode is needed.

or... if there is a back button then you don't need to move history view into edit mode.

  • The user clicks back (unsaved changes stored),
  • goes to history,
  • views the history (which includes unsaved changes at the top),
  • goes back to edit (resume unsaved changes)
  • and they are working where they left off.

We now have opened up all the rest of the UI while still in in midst edit so this is more powerful than just the closing the browser accidentally use-case.

Later an indicator can be added to show you have unsaved changes on both the toolbar and the contents view and a list of what you recently left unsaved. But that's not needed now.

Add "autosave" to Vale's Plone dictionary.
@stevepiercy
Copy link
Collaborator

stevepiercy commented Sep 27, 2023

I replaced "load" with "restore" and added "autosave" to Vale to silence spell check errors.

@stevepiercy
Copy link
Collaborator

I fixed a couple of i18n issues, but I have no idea of how to fix the remaining few. https://github.com/plone/volto/actions/runs/6322673648/job/17168722091?pr=4252

Is that a release manager task?

@sneridagh sneridagh modified the milestones: Plone 6.1, 18.x.x Sep 30, 2023
@mister-roboto
Copy link

rexalex, ALEXANDRU MEDESAN your emails are not known to GitHub and thus it is impossible to know if you have signed the Plone Contributor Agreement, which is required to merge this pull request.

Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement How to add more emails to your GitHub account: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/adding-an-email-address-to-your-github-account

Copy link

netlify bot commented Mar 2, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 4b972da
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/65e367ef79f3430008428248

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs discussion
Development

Successfully merging this pull request may close these issues.

Auto-Save option