💻 autosave feature #5311

hasan-sh · 2024-03-25T16:03:53Z

Note
The autoSave function that i created can be used generically. So, in case we need it in the future, these are the steps that are needed:

make sure that in the HTML you have a form element with a onsubmit event (or htmx!).
- all the inputs elements (incl. select, textarea etc) are then handled by the function
then add a script that invokes the function with the id of that form element, as in https://github.com/hedyorg/hedy/blob/autosave/templates/profile.html#L184-L189.
the fuction that you use to make the api request will be invoked once a change has occurred.

Furthermore, sometimes there's an element that we cannot attach an event listener on, like the CKEditor. In this case, you can pass an event as a secon argument to the autoSave function. When you do this, we simply invoke the submit trigger on the form element.

The customize-class page has two differently implemented features. The sortable adventures and the other inputs below. These two have to be syncronized. If we hide all quizzes, for instance, we want the adventure tabs to be updated. Now, instead of changing the whole structure, a third argument could be passed indicating that we should trigger an event on a certain element. See the autosave.ts file on how it's used.

How to test?
check the description of #4888 and test these pages.

- debounced every 1min

Felienne

Nice work @hasan-sh!

A few notes:

The (i) text appears in a weird place:

(not sure that is caused by this PR?)

The back button does not work anymore:

results in:

For the password save, I doubt whether we want an autosave? Somehow that (like making a profile public) feels like a very deliberate action. What do you think?

hasan-sh · 2024-04-01T12:56:06Z

The (i) text appears in a weird place:

Which one exactly?

The back button does not work anymore:

Correct! I think @Annelein was going to fix this issue after adding all go back buttons?

For the password save, I doubt whether we want an autosave? Somehow that (like making a profile public) feels like a very deliberate action. What do you think?

I agree, shall i remove it?

Annelein · 2024-04-01T12:58:55Z

Correct! I think @Annelein was going to fix this issue after adding all go back buttons?

See #5263

hasan-sh · 2024-04-01T14:13:22Z

Great, that's now solved by #5349

Felienne · 2024-04-01T14:31:09Z

The (i) text appears in a weird place:

The one I pasted above shows all the way at the bottom:

Should be above with the (i) I guess?

jpelay

Hi @hasan-sh! This is a very cool feature, and I love it. I just had a couple of comments about code and functionality. Thanks!

jpelay · 2024-04-09T16:10:32Z

static/js/autosave.ts

+   ) {
+  // Debounce function to prevent excessive requests
+    const formElement = document.getElementById(formId) as HTMLFormElement;
+    console.log(formId, formElement, customEvent)


Opps, can you delete this debug console log?

Good catch, thanks will do:)

jpelay · 2024-04-09T16:10:51Z

static/js/autosave.ts

+const DEBOUNCE_TIMEOUT = 1000;
+
+let timeoutId: ReturnType<typeof setTimeout> | null = null;
+function debounce<F extends (...args: any[]) => any> (func: F, delay: number): (...args: Parameters<F>) => void {


I really like this solution!

jpelay · 2024-04-09T16:12:09Z

templates/customize-adventure.html

@@ -209,4 +210,13 @@ <h3>{{_('adventure_exp_classes')}}</h3>
        </div>
    </div>
 </div>
+<script>
+    document.addEventListener("DOMContentLoaded", () => {


A while ago we were trying to get away from adding scripts to the templates, because we wanted all javascript code to be in a single place. I think you could use initiliaze.ts to initialize these functions in JavaScript.

Why though? I mean we don't really need to initialize anything if the route is not in the wanted page! What I do know is that the main issues with having different scripts is that it could cause a sluggish experience. Was there another (specific) problem we faced? I see that we still use it in some places!!

We could put it in initialize.ts, but it's not as incapsulated as this is! Perhaps it's a matter of docummentation?

I think the reason is that if all code is in one place, it is easy to see what is there. In the past we ran into the issue that similar code was in multiple places and when something was changed, some locations were overlooked. People might not look for js inside an HTML file (it is a sort of separation of concerns)

So I support the idea of having as much in one place as possible!

I agree about the separation of cencerns concern. Will change it.

Two points. First, so the current scripts that are in html files we still have to clean up? And second there's several downsides of our current approach. The main and crucial one is that gradually more and more TS must be loaded even though it's not in use yet. This affects the overall performance of our app. If you're satisfied with the overall performance, we tackle it when needed. Just good to keep it in mind i think!

Two points. First, so the current scripts that are in html files we still have to clean up?

Yes, I would assume so! There is a lot of places where we still have js/ts in the HTML files, and we like to move that to HTMX or to a typescript file. I don't necessarily think we should do all of that right away, but maybe if you encounter it while working on other things, you can try to clean it up piece by piece. You can also do a bit of cleanup in between other tasks if you think it is a fun palet cleanser!

The main and crucial one is that gradually more and more TS must be loaded even though it's not in use yet.

I don't think that is where the bottle neck of performance is, but yes let's keep it in mind!

jpelay · 2024-04-09T16:14:11Z

templates/profile.html

Is there a way to prevent this page from reloading every time I click on one of the options? The autosave works great, but that makes this page a little bit annoying to use.

Which options do you mean? I see it only reloads if we change the language or keyword language! These require a reload since the Ui changes accordingly!

hasan-sh · 2024-04-10T11:21:41Z

Should be good to go @jpelay !!

jpelay

Perfect!

Everything's done!

mergify · 2024-04-10T13:29:16Z

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

hasan-sh and others added 18 commits March 25, 2024 16:40

- a generic function for autosaving

b12d83b

- debounced every 1min

autosave adventure

c5ceebd

autosave with custom events

66ae653

autosave class page

47b0043

autosave profile page

874b5c1

bundles

9c53265

remove save buttons

baaf8b2

Merge branch 'main' into autosave

f18d2df

🤖 Automatically update generated files

10d6365

do not trigger update here

868b686

fix tests

888bdb0

Merge branch 'main' into autosave

139196f

bundle file

ab87a3d

tests

59c4313

Merge branch 'main' into autosave

27d539a

🤖 Automatically update generated files

e02ec78

test dev mode with new class

07077e1

Merge branch 'autosave' of github.com:hedyorg/hedy into autosave

5096f50

Felienne previously requested changes Apr 1, 2024

View reviewed changes

Felienne added the under-review label Apr 1, 2024

hasan-sh and others added 3 commits April 2, 2024 12:11

password needs confirmation.

2a76bea

Merge branch 'main' into autosave

d27d70a

🤖 Automatically update generated files

2f3597a

Felienne assigned jpelay Apr 2, 2024

hasan-sh mentioned this pull request Apr 2, 2024

> The info text that appears by clicking the (i) button should be replaced. #5356

Closed

Merge branch 'main' into autosave

68fb2fb

more robust tests

e9e71bc

jpelay requested changes Apr 9, 2024

View reviewed changes

add autosave function in respective files.

7079377

hasan-sh and others added 2 commits April 10, 2024 13:23

Merge branch 'main' into autosave

57ef4b3

🤖 Automatically update generated files

3dedc47

jpelay approved these changes Apr 10, 2024

View reviewed changes

mergify bot merged commit d3f9d18 into main Apr 10, 2024
12 checks passed

mergify bot deleted the autosave branch April 10, 2024 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💻 autosave feature #5311

💻 autosave feature #5311

hasan-sh commented Mar 25, 2024

Felienne left a comment

hasan-sh commented Apr 1, 2024

Annelein commented Apr 1, 2024

hasan-sh commented Apr 1, 2024

Felienne commented Apr 1, 2024

jpelay left a comment

jpelay Apr 9, 2024

hasan-sh Apr 10, 2024

jpelay Apr 9, 2024

jpelay Apr 9, 2024

hasan-sh Apr 10, 2024

Felienne Apr 10, 2024

hasan-sh Apr 10, 2024

Felienne Apr 10, 2024

jpelay Apr 9, 2024

hasan-sh Apr 10, 2024

hasan-sh commented Apr 10, 2024

jpelay left a comment

mergify bot commented Apr 10, 2024

💻 autosave feature #5311

💻 autosave feature #5311

Conversation

hasan-sh commented Mar 25, 2024

Felienne left a comment

Choose a reason for hiding this comment

hasan-sh commented Apr 1, 2024

Annelein commented Apr 1, 2024

hasan-sh commented Apr 1, 2024

Felienne commented Apr 1, 2024

jpelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasan-sh commented Apr 10, 2024

jpelay left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 10, 2024