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

Saving the playground state in local storage #1942

Merged
merged 5 commits into from
May 28, 2024

Conversation

mani-chand
Copy link
Contributor

@mani-chand mani-chand commented Mar 25, 2024

#1476 issue. Updated the function call(getCodeFromPlayground) in save-playground.js file from pagehide event to change event in ace editor.

@mani-chand mani-chand changed the title Editor change event is used to save the state for saving the code in local storage. saving the playground state in local storage. Mar 25, 2024
@mani-chand
Copy link
Contributor Author

mani-chand commented Mar 25, 2024

Hello @djmitche, @mgeisler, Look at the changes made.

@djmitche djmitche self-requested a review April 1, 2024 13:53
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I'd like @mgeisler to take a look and try it out before we merge again.

@djmitche djmitche requested a review from mgeisler April 1, 2024 14:00
Comment on lines 26 to 28
editor.session.on("change", function () {
let code = editor.getValue();
codes.push(code);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @mani-chand, thanks for looking more at this!

Will this not push the text into codes on every change to any editor on the page? That sounds wrong to me: I would expect the change event to update a single item in the codes list.

More importantly, how have you tested this? It was a little hard to trigger the problem with the old code, so it's important that we find a way to improve the testing of any new code.

That will probably be something simple like "I ran mdbook serve --open, and browsed back and forth 10 times between two pages". If that "procedure" can reproduce the old error, then we can use it to test the new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @mgeisler , I think, I did the changes you wanted. Could you verify them and suggest any changes if needed.

About testing, In old code , we used javascript event listener to save the code , now, we are using ace editor's change event this time it won't be a problem.

I did normally checked in my local , It worked good for me . Just check at your local and let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @mani-chand, thanks for the update, I'll take a look in a minute.

An important first question is: are you able to reliably reproduce the error before this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, But i think the error came because of closing window/tab directly changing the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mani-chand, thanks for the update, I'll take a look in a minute.

An important first question is: are you able to reliably reproduce the error before this fix?

But if you want me to do, I will try and reproduce the error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I am not able to replicate it.

Okay, that is interesting 🙂

The problem I saw with the old code was that the pages would become empty after a while. I noticed this while browsing the rendered book — the next day some of the students told me that they had also noticed the missing code blocks.

If we cannot reproduce the old error, then I don't feel confident in a PR that tries to fix it. The problem is that we won't know if the PR is really fixing the error if we don't know how to reproduce the error.

It would be helpful if you can figure out how to reliably reproduce the error and then test the same procedure on the new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I am not able to replicate it.

Okay, that is interesting 🙂

The problem I saw with the old code was that the pages would become empty after a while. I noticed this while browsing the rendered book — the next day some of the students told me that they had also noticed the missing code blocks.

If we cannot reproduce the old error, then I don't feel confident in a PR that tries to fix it. The problem is that we won't know if the PR is really fixing the error if we don't know how to reproduce the error.

It would be helpful if you can figure out how to reliably reproduce the error and then test the same procedure on the new code.

Hello , It may take time to complete this because of my final exams.

Copy link
Contributor Author

@mani-chand mani-chand Apr 22, 2024

Choose a reason for hiding this comment

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

Hello @mgeisler , Found the bug 🎉 . All non-editable exercise playground codes went missing/cleared . It is may be because of ACE API i am trying to replace the playground code with the same code in non-editable playground it get cleared. (because of pagehide event excercise code will save in localstorage and when we visit the exercise again savePlayground function will try to replace the same code but because of non-editable playground code will get cleared).

Can you please verify that and confirm me.

Copy link
Contributor Author

@mani-chand mani-chand Apr 22, 2024

Choose a reason for hiding this comment

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

Hello @mgeisler , Found the bug 🎉 . All non-editable exercise playground codes went missing/cleared . It is may be because of ACE API i am trying to replace the playground code with the same code in non-editable playground it get cleared. (because of pagehide event excercise code will save in localstorage and when we visit the exercise again savePlayground function will try to replace the same code but because of non-editable playground code will get cleared).

Can you please verify that and confirm me.

To reproduce the issue :

Go to a non - editable playground and refresh the page
or
Go to a non - editable playground and go to next page and visit again.

@mani-chand mani-chand requested a review from mgeisler April 4, 2024 17:13
@mgeisler mgeisler changed the title saving the playground state in local storage. Saving the playground state in local storage Apr 8, 2024
@mani-chand
Copy link
Contributor Author

Hello @mgeisler, Can you give me a update on it.

@mgeisler
Copy link
Collaborator

Hello @mgeisler, Can you give me a update on it.

Hi @mani-chand, I haven't forgotten about this — I spent some time looking at it today and I think you're right that there is a problem with the pages that have non-editable playgrounds. However, I don't understand what the problem is... I don't see any exceptions in the JavaScript console, but I do see the code block disappear.

This makes me nervous about changing this code more. I will look more next week!

@mani-chand
Copy link
Contributor Author

Hello @mgeisler, Can you give me a update on it.

However, I don't understand what the problem is... I don't see any exceptions in the JavaScript console, but I do see the code block disappear.

Hello @mgeisler , its because of pagehide event excercise code will save in localstorage and when we visit the exercise(non editable playground) again savePlayground function will try to replace the same code but because of non-editable playground code will get cleared.

Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with it! I tested it locally and it works really well 🎉 Let us get it merged and out into the hands of people so we can get it tested widely.

@mgeisler mgeisler enabled auto-merge (squash) May 28, 2024 09:48
@mgeisler mgeisler merged commit e2cad7d into google:main May 28, 2024
33 checks passed
djmitche added a commit to djmitche/comprehensive-rust that referenced this pull request May 29, 2024
djmitche added a commit that referenced this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants