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

Remember input in LocalStorage #1382

Merged
merged 1 commit into from Nov 8, 2023
Merged

Remember input in LocalStorage #1382

merged 1 commit into from Nov 8, 2023

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Oct 15, 2022

closes #456

@hamza221
Copy link
Contributor Author

@jotoeri @Chartman123 Can you please help figuring out how to change QuestionDate State on mounted() :)

@hamza221 hamza221 added enhancement New feature or request help wanted Extra attention is needed 2. developing Work in progress feature: 📝 submitting responses labels Oct 15, 2022
@jotoeri
Copy link
Member

jotoeri commented Oct 15, 2022

Nice start!
However, i need to write my Masters Thesis currently, so i'm sorry, but i'm unfortunately not able to dig into until mid/end of November. Just aftwerwards i could help you...

@jancborchardt
Copy link
Member

@Chartman123 @susnux @skjnldsv maybe you can help? :)

@jotoeri all the best for the thesis! :)

@Chartman123
Copy link
Collaborator

I will try to find some time after the weekend :)

@susnux
Copy link
Collaborator

susnux commented Oct 16, 2022

Nice start on this one!

how to change QuestionDate State on mounted?

Currently this is not possible, as Dropdown and Date are not initialized from any property but start blank (whereas the other types are already initialized by the values property).

I filed a PR for initializing those question types from the values property as well ( #1383 ), which should make this more easy.


I did not have a in depth view on your changes, but one thing I was thinking about is:
What happens if you have multiple forms with saved states?

@hamza221
Copy link
Contributor Author

@susnux Hey good catch with the multiple forms issue, I didn't pay attention to this scenario. I'll update the PR and Fix the failing Checks.

@hamza221 hamza221 self-assigned this Oct 18, 2022
@hamza221 hamza221 changed the title Remember input in LocalStorage fix #456 Remember input in LocalStorage May 18, 2023
@hamza221 hamza221 added 3. to review Waiting for reviews and removed help wanted Extra attention is needed 2. developing Work in progress labels May 18, 2023
Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Hi @hamza221! Nice to see you back here 🙂

I don't know much about LocalStorage. However, I think that this is not working as it's intended. I can only see the stored values as long as I navigate within the same form (e.g. between submit - edit - submit). As soon as I leave the form and go to another one and then come back all fields are empty again. As soon as I then do a reload on the page, the values are loaded.

Also could you please do a rebase on current main as we have quite some newer versions in composer.lock 👍🏻

src/views/Submit.vue Outdated Show resolved Hide resolved
src/views/Submit.vue Outdated Show resolved Hide resolved
src/views/Submit.vue Outdated Show resolved Hide resolved
src/views/Submit.vue Outdated Show resolved Hide resolved
@hamza221
Copy link
Contributor Author

hamza221 commented May 19, 2023

Thank you @Chartman123

As soon as I leave the form and go to another one and then come back all fields are empty again

The problem here is that the init from localhost is done in beforeMount hook
and apparently all the forms are mounted to the Dom from the beginning we can either change the mounting/unmounting logic or find a work around. what do you suggest?
but for now it works perfectly in public view

Solved

@Chartman123
Copy link
Collaborator

Chartman123 commented May 22, 2023

On a public share link this.hash and this.shareHash seem to be empty, so the storage key is then nextcloud_forms_.
grafik
But I don't know how to add the hash of the form in this case.

Furthermore the array still has lots of null values. Probably because you add the values to the local storage with this.formValuesForLocalStorage[question.id]. But I don't know if it's possible to do this another way. It just makes the LocalStorage hard to read in the browser console, but as long as it works correctly I wouldn't mind too much.

@jancborchardt Should we also add a "Reset Form" button to clear the LocalStorage and start fresh?

@Chartman123 Chartman123 added this to the 3.4 milestone Jun 26, 2023
@hamza221
Copy link
Contributor Author

The null, and empty hash are solved, @jancborchardt should we add a Reset Form button?

@Chartman123
Copy link
Collaborator

@hamza221 Looks way better now :) However, when you switch the form within one session you end up with multiple records with duplicated contents:
grafik

Looks like we need to reset the object between opening different forms so that the old contents don't get written to the content of another form. I think we need to move the init function away from "beforeMount".

Also, I think that we can get rid of the "id" key inside the object as you're already using it as a key in the top object.

And please rebase on current main 🙂

@hamza221
Copy link
Contributor Author

hamza221 commented Aug 6, 2023

same job as #1416 failing :/

@Chartman123
Copy link
Collaborator

The failed jobs must be something general with the images... I also tried to use the same as in the mail repo, but that didn't help either.

Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Looks good now to me. 👍🏻

@susnux
Copy link
Collaborator

susnux commented Aug 16, 2023

Basically this is good now 👍

But there is one thing I am unsure about: Do we want to store values on public shares for anonymous users?
As there might be places where multiple users use the same device, in this case on could see the input of the previous one. (just came to my mind, what do you think?)

@Chartman123
Copy link
Collaborator

Yes, that's a valid point.

What about combining this here with my started PR regarding the warning when you leave an unsubmitted form? So that we only store the values to the local storage when the user is leaving an unsubmitted form and chooses to save them?

@Chartman123
Copy link
Collaborator

@hamza221 Submit.vue now handles leaving the form with unsubmitted changes. You could try to update this PR so that data will only be stored in local storage when the user leaves the form without a submission and answers the dialog accordingly

@hamza221
Copy link
Contributor Author

hamza221 commented Oct 23, 2023

@Chartman123 I'm not sure I understand how this will fix @susnux 's concern. So if I understand correctly ->

  1. User attempts to leave unsubmitted form
  2. He gets a warning
  3. clicks on Leave page
  4. we save his answers in local storage
    => this case we can still tun into what @susnux talked about

Or do you mean we save it when he chooses to stay? If so I'm not sure how is that useful.

What about saving the answers only if the user is connected and showing the warning otherwise?

I'm sorry I'm just trying to figure out what's the most useful path we should take :))
I'm gonna check how google forms does it

Update: Google Forms only saves for logged in users and only shows Warning for anonymous users
Update 2 : the saved data works cross browser, it's not saved in local storage afaik, So in our case I think the warning is still necessary for logged in users unless we decide to save unsubmitted entries in the db and discard this PR

@Chartman123
Copy link
Collaborator

@hamza221 I meant something like this:

  1. User has unsubmitted data and leaves the form
  2. Popup message/modal: "You have unsaved data. Do you want to save before you leave?"
  3. Yes -> trigger save, no -> leave without saving

What about saving the answers only if the user is connected and showing the warning otherwise?
[...]
Google Forms only saves for logged in users and only shows Warning for anonymous users

In this case we could also store the data to the database instead of local storage (see your second update) with the benefit of being independent of the browser. However, this is of course more work as we will have to adjust the API/backend as well.

@susnux @jancborchardt @jotoeri What do you think?

@susnux
Copy link
Collaborator

susnux commented Oct 23, 2023

I would prefer to go with you suggested solution but only for logged-in users

User has unsubmitted data and leaves the form
Popup message/modal: "You have unsaved data. Do you want to save before you leave?"
Yes -> trigger save, no -> leave without saving

@susnux
Copy link
Collaborator

susnux commented Oct 23, 2023

Saving in DB is possible, but might be quite complex. As it sounds a bit like the "allow users to edit their answers" feature.
So could be implemented by adding a flag to answers like "isDraft" and if yes that answer is not considered for the results.

Nevertheless I think we should go with @hamza221 PR here for the moment and just do not save for anonymous users.

@jancborchardt
Copy link
Member

jancborchardt commented Oct 24, 2023

Yeah, it's better to have localStorage saving now for error prevention than work longer to get it into the database. The main case is accidentally closing tabs or browser crashes. :)

@hamza221
Copy link
Contributor Author

hamza221 commented Nov 7, 2023

Chrome Safari
image image

|

confirm(t('forms', 'You have unsaved changes! Do you still want to leave?'))
The confirmation text is not matching, moreover as far as I understand even if we manage to fix the text and make it

You have unsaved data. Do you want to save before you leave?

Edit: custom messages are unsupported on all major browsers
The buttons' text can't be changed to save , leave for eg
suggestion: keep default warning for all users and save automatically for logged in users only

@Chartman123
Copy link
Collaborator

@hamza221 So as far as I can see you already implemented that we will only store and restore the answers to/from the LocalStorage if a user is logged in. This would fix the original issue and so for me it's fine. We can try to combine it with the leave message in a follow-up PR.

confirm(t('forms', 'You have unsaved changes! Do you still want to leave?'))
The confirmation text is not matching, [...]

This could be implemented in beforeRouteLeave() and beforeRouteUpdate()

I will do the code review and testing today :) Thank you very much!

src/views/Submit.vue Outdated Show resolved Hide resolved
@hamza221
Copy link
Contributor Author

hamza221 commented Nov 8, 2023

This could be implemented in beforeRouteLeave() and beforeRouteUpdate()

I'm not sure how . Which modal would you use. I don't think confirm() or alert () are adequate for this use case and I'm not sure how to make a custom modal blocking.

@Chartman123
Copy link
Collaborator

I'm not sure how . Which modal would you use. I don't think confirm() or alert () are adequate for this use case and I'm not sure how to make a custom modal blocking.

All fine :) Let's move to a follow-up 👍🏻

src/views/Submit.vue Outdated Show resolved Hide resolved
Signed-off-by: hamza221 <hamzamahjoubi221@gmail.com>
@Chartman123 Chartman123 merged commit a6fb2d5 into main Nov 8, 2023
22 checks passed
@Chartman123 Chartman123 deleted the fix/remember-input branch November 8, 2023 17:08
@Chartman123
Copy link
Collaborator

@hamza221 Could you please have another look at the code: I missed a problem when you reload a form with saved values: the UI doesn't update when you choose an option of a checkbox/radio question...

@Chartman123
Copy link
Collaborator

@susnux @hamza221 The problem looks a lot like the problem that we fixed here for "other" answer questions... #1764

Might this also come from having :values.sync... and @update:values... in Submit.vue for the Questions?

@Chartman123
Copy link
Collaborator

Created a new issue for the problem: #1780

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to come back to a partially filled form
5 participants