-
-
Notifications
You must be signed in to change notification settings - Fork 702
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for volto ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@sneridagh Take a look at the UX, let me know how it feels to you. How to test:
|
Some things to discuss:
|
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.
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.
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.
I cannot think of any scenario where it would be useful to disable autosave. Can you think of a common use case? |
@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.
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. |
@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". |
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? In that screenshot, I created a News content type at 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 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. |
@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 |
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:
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. |
@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. |
@tisto Something like that. I've actually tested with the "discard my saved form when I click cancel", it's this commented line: volto/src/components/manage/Form/Form.jsx Line 439 in 4fd716a
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". |
This needs a check if the page has been edited since, doesn't it? |
@ksuess Indeed, it would be a good idea to check last modification times, in case another user edited a document. |
@tiberiuichim Thanks for starting work on this!
I like this idea. Do we need any special considerations for how it interacts with edit locking? |
@davisagli no, it all happens client side based on saved data in the localstorage. The current internal behavior is like this:
As the key for the localstorage, we use this algorithm:
|
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". |
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 |
Recovery is not the right term because the content was never lost. The autosaved content is a version of stored content. |
@tisto @stevepiercy How about "local autosave" to clarify it's not being persisted in server? |
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. |
@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: 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). |
@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. |
@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 |
@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! |
@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. |
Volto Team Meeting (2023-09-26) decisions:
|
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:
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.
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.
I replaced "load" with "restore" and added "autosave" to Vale to silence spell check errors. |
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? |
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 |
✅ Deploy Preview for plone-components canceled.
|
Fixed #4168