plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 576 forks source link

Save edit form as draft using localStorage #4252

Open tiberiuichim opened 1 year ago

tiberiuichim commented 1 year ago

Fixed #4168

netlify[bot] commented 1 year ago

Deploy Preview for volto ready!

Name Link
Latest commit 4b972da6cf40fabce291359ed4c0e4470f7e00c0
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 commented 1 year ago

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

How to test:

tiberiuichim commented 1 year ago

Some things to discuss:

stevepiercy commented 1 year ago
  • 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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

https://user-images.githubusercontent.com/141568/212013483-4e644e07-03b4-441b-85de-9f2b2df103a9.mp4

stevepiercy commented 1 year ago

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:

  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 commented 1 year ago

@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 commented 1 year ago

@tisto Something like that. I've actually tested with the "discard my saved form when I click cancel", it's this commented line: https://github.com/plone/volto/blob/4fd716a7071be65e839333dc89e91534afae5e12/src/components/manage/Form/Form.jsx#L439

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 commented 1 year ago
  • you'll see a "Autosave found, load it?"

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

tiberiuichim commented 1 year ago

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

davisagli commented 1 year ago

@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 commented 1 year ago

@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:

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 commented 1 year ago

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

tiberiuichim commented 1 year ago

@stevepiercy the "form unload handler", plone classic has it. We can model behavior based on that one, though this implementation is kind of a substitution for that one. Maybe we can combine them.

davisagli commented 1 year ago

@tiberiuichim I think what I meant about locking was the same point Katja raised about making sure the saved draft can't overwrite a more recent change. Sounds like you have a plan for that.

@stevepiercy It seems to me that if this autosave exists, then it's not necessary to show a "Are you sure you want to destroy your work?" message, because your work is not going to be destroyed even if you leave the page. I guess we could show that message, but it might be annoying when you're actually leaving the page intentionally.

cypress[bot] commented 10 months ago

Passing run #6705 ↗︎

0 565 20 0 Flakiness 0

Details:

chore: merge develop
Project: Volto Commit: 1dea385a40
Status: Passed Duration: 18:01 💡
Started: Aug 2, 2023 5:53 AM Ended: Aug 2, 2023 6:11 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

rexalex commented 10 months ago

Hello, I took over the feature from @tiberiuichim.

Current version has:

rexalex commented 10 months ago

Demo for how autosave removes storage on Save form or Cancel load saved data:

https://user-images.githubusercontent.com/7466122/247877147-d83070e9-4c47-4d1a-85f5-908d8839684d.mov

rexalex commented 10 months ago

Demo for how autosave manages stale data (content gets edited more recently than the local saved data):

https://user-images.githubusercontent.com/7466122/247877882-5fc3efb3-cf3e-4abc-a649-0c78a8a89d16.mov

rexalex commented 10 months ago

Demo for how autosave adds new content type:

https://user-images.githubusercontent.com/7466122/247878398-7eb4e22b-f748-4fa4-8254-451e69949738.mov

rexalex commented 10 months ago

Demo for how autosave handles accidental refresh in edit and add modes:

https://user-images.githubusercontent.com/7466122/247879058-1c7d1954-111e-468f-b456-198e3bfdd9b0.mov

rexalex commented 10 months ago

Demo for comments autosave:

https://user-images.githubusercontent.com/7466122/249213695-16ac7fa1-b3a2-44f2-929e-69e382b8c08f.mov

stevepiercy commented 10 months ago

@rexalex thank you for picking up this PR.

This PR will need docs to describe what this new killer feature is and does.

Videos are good, but they need to be shown in a narrow portal of no more than 743 pixels and embedded into docs per https://6.docs.plone.org/contributing/documentation/myst-reference.html#video.

I am not certain where docs should go for this feature. It might be a new page. I think a new page would be good because it would boost SEO for the autosave feature of Volto, a truly "killer feature" amongst CMSs. I also think this feature is primarily for the user who edits content, not so much the developer. I think it belongs in the User Manual. What do y'all think?

stevepiercy commented 9 months ago

Also add an entry to docs/source/user-manual/index.md for autosave.

stevepiercy commented 9 months ago

@rexalex please resolve merge conflicts with master branch.

stevepiercy commented 9 months ago

@rexalex please resolve merge conflicts with master branch.

rexalex commented 8 months ago

@stevepiercy done

stevepiercy commented 8 months ago

@tiberiuichim @sneridagh @davisagli @ksuess this is ready for review.

sneridagh commented 7 months ago

@rexalex @tiberiuichim some minor text tweaks and let's merge it!

stevepiercy commented 7 months ago

@rexalex @tiberiuichim it looks like the autosave tests need to be updated to reflect the changes in the messages, too.

tisto commented 7 months ago

@sneridagh I get an error when trying to build this branch locally (backend):

An internal error occurred due to a bug in either zc.buildout or in a
recipe being used:
Traceback (most recent call last):
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/buildout.py", line 2252, in main
    getattr(buildout, command)(args)
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/buildout.py", line 856, in install
    installed_files = self[part]._call(recipe.install)
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/buildout.py", line 1652, in _call
    return f()
  File "/Users/timo/workspace/dotfiles/.buildout/eggs/zc.recipe.testrunner-3.0-py3.8.egg/zc/recipe/testrunner/__init__.py", line 46, in install
    eggs, ws = self.egg.working_set(('zope.testrunner', ))
  File "/Users/timo/workspace/dotfiles/.buildout/eggs/zc.recipe.egg-2.0.7-py3.8-macosx-13.3-arm64.egg/zc/recipe/egg/egg.py", line 78, in working_set
    ws = self._working_set(
  File "/Users/timo/workspace/dotfiles/.buildout/eggs/zc.recipe.egg-2.0.7-py3.8-macosx-13.3-arm64.egg/zc/recipe/egg/egg.py", line 161, in _working_set
    ws = zc.buildout.easy_install.install(
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 971, in install
    return installer.install(specs, working_set)
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 717, in install
    req = self._constrain(current_requirement)
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 664, in _constrain
    requirement = _constrained_requirement(constraint,
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 1641, in _constrained_requirement
    if version not in requirement:
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/pkg_resources/__init__.py", line 3201, in __contains__
    return self.specifier.contains(item, prereleases=True)
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/pkg_resources/_vendor/packaging/specifiers.py", line 902, in contains
    item = Version(item)
  File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/pkg_resources/_vendor/packaging/version.py", line 197, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
pkg_resources.extern.packaging.version.InvalidVersion: Invalid version: '5.3.1 # without this volto is not building locally'
make: *** [.installed.cfg] Error 1
make: *** Deleting file `.installed.cfg'

I guess this is not related to this PR at all...

sneridagh commented 7 months ago

I almost never use the local build anymore… I always spin a container for the backend. I think Alok found this the other day in a PR, could be that it didn’t get merged yet. It was about a dep (again) in buildout.

On Sat, 16 Sep 2023 at 09:56, Timo Stollenwerk @.***> wrote:

@sneridagh https://github.com/sneridagh I get an error when trying to build this branch locally (backend):

An internal error occurred due to a bug in either zc.buildout or in a recipe being used: Traceback (most recent call last): File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/buildout.py", line 2252, in main getattr(buildout, command)(args) File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/buildout.py", line 856, in install installed_files = self[part]._call(recipe.install) File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/buildout.py", line 1652, in _call return f() File "/Users/timo/workspace/dotfiles/.buildout/eggs/zc.recipe.testrunner-3.0-py3.8.egg/zc/recipe/testrunner/init.py", line 46, in install eggs, ws = self.egg.working_set(('zope.testrunner', )) File "/Users/timo/workspace/dotfiles/.buildout/eggs/zc.recipe.egg-2.0.7-py3.8-macosx-13.3-arm64.egg/zc/recipe/egg/egg.py", line 78, in working_set ws = self._working_set( File "/Users/timo/workspace/dotfiles/.buildout/eggs/zc.recipe.egg-2.0.7-py3.8-macosx-13.3-arm64.egg/zc/recipe/egg/egg.py", line 161, in _working_set ws = zc.buildout.easy_install.install( File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 971, in install return installer.install(specs, working_set) File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 717, in install req = self._constrain(current_requirement) File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 664, in _constrain requirement = _constrained_requirement(constraint, File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/zc/buildout/easy_install.py", line 1641, in _constrained_requirement if version not in requirement: File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/pkg_resources/init.py", line 3201, in contains return self.specifier.contains(item, prereleases=True) File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/pkg_resources/_vendor/packaging/specifiers.py", line 902, in contains item = Version(item) File "/Users/timo/workspace/plone/volto/api/lib/python3.8/site-packages/pkg_resources/_vendor/packaging/version.py", line 197, in init raise InvalidVersion(f"Invalid version: '{version}'") pkg_resources.extern.packaging.version.InvalidVersion: Invalid version: '5.3.1 # without this volto is not building locally' make: [.installed.cfg] Error 1 make: Deleting file `.installed.cfg'

I guess this is not related to this PR at all...

— Reply to this email directly, view it on GitHub https://github.com/plone/volto/pull/4252#issuecomment-1722169318, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADW4D367JK5UPXVE6URW73X2VLT3ANCNFSM6AAAAAATYIBNDE . You are receiving this because you were mentioned.Message ID: @.***>

tisto commented 7 months ago

@sneridagh ok, I gave this a shot locally. When I click on the "yes" icon in the modal nothing happens and I see an error trace in the JS console:

Screenshot 2023-09-16 at 16 47 56

Some other observations:

Screenshot 2023-09-16 at 16 49 23
sneridagh commented 7 months ago

@sneridagh ok, I gave this a shot locally. When I click on the "yes" icon in the modal nothing happens and I see an error trace in the JS console:

Screenshot 2023-09-16 at 16 47 56

Some other observations:

  • as @tiberiuichim suggested, we might want to not show the autosave modal when the user intention was to drop the content and click on the "x" cancel option

You are proposing to do not autosave when the user intention was "cancel", thus discard all the changes made until that moment, right? I'm +1 to change the flow in this way.

  • the modal shows up very briefly, I'd say this is important enough to stay

I agree, it should be permanent, I'd say, until the user makes up his mind.

  • as a user I am clueless what my latest changes were, when I have no way of seeing the differences, it might be very tough decision, especially if I am not sure if the system would replace my current version with the restored version. the question that arises is: do I have to save afterwards or do I make a final decision here

As one of the videos shows, the user can undo-redo the changes to see them. The decision is not final, and you can always not save, load again the server version. And if for some reason someones changes its mind after save, then you can revert the change in the history view.

  • the go/cancel icons are too far from each other and the "x" right to those buttons is confusing, we show two different "x" icons to the user, this needs an UI overhaul

I agree, any proposal?

Screenshot 2023-09-16 at 16 49 23
  • I'd also improve the modal text explaining what happened and what the options are, maybe we need to be more explicit here and we shouldn't use the Volto save/cancel buttons at all

Any suggestion? Steve already reviewed that and it seemed to him enough explicit. At the end the options are not that complicated, apply the saved changes or not. Saying I don't think is right or correct is not enough.

sneridagh commented 7 months ago

@rexalex could you please take care of the things mentioned above, and the error @tisto found? It seems an easy one, a rendering of a non allowed element inside a <p>.

tisto commented 7 months ago

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 commented 7 months ago

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

sneridagh commented 7 months ago

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

stevepiercy commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

djay commented 7 months ago

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:

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.

stevepiercy commented 7 months ago

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