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

Implemented autosave on a 5-second timer #3037

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Levi-Lesches
Copy link

Fixes #2871 by saving the editor content to local storage under they key user_input every 5 seconds, and in dispose


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@Levi-Lesches
Copy link
Author

@parlough I fixed the double quotes

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks so much for implementing this again :D

pkgs/dartpad_ui/lib/editor/editor.dart Outdated Show resolved Hide resolved
Comment on lines 281 to 282
fallbackSnippet: web.window.localStorage.getItem('user_input') ??
Samples.getDefault(type: 'dart'))
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth it to convert this fallback to a callback, so we don't unnecessarily retrieve the old snippet?

Copy link
Author

@Levi-Lesches Levi-Lesches Aug 16, 2024

Choose a reason for hiding this comment

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

I think ?? is short circuiting, so it should be fine? EDIT: Just checked, it is

Copy link
Author

Choose a reason for hiding this comment

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

Oh, if you meant not retrieving the old snippet or user input in case a gist id or otherwise is provided.

I tried making it a callback, and got some very strange errors on the web side. Each time I restarted, it was that JS can't read some property, but it was a different property each time. Once it didn't even let the flutter tool connect because the bootstrapper got stuck. Not sure what's up with that but I'll leave the PR as-is for now

Copy link
Author

Choose a reason for hiding this comment

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

@parlough I Implemented this in the latest set of commits. I think there was something wrong with my Flutter installation last time I was working on this. Just upgraded to 3.27.0 with no issues.

@Levi-Lesches
Copy link
Author

@parlough Any updates?

@Levi-Lesches
Copy link
Author

Sorry for the annoying pings, just want to make sure this doesn't slip between the cracks

@parlough
Copy link
Member

@Levi-Lesches Sorry for the delay, I've been away.

The change looks good to me, but before landing, we need to wait for others to determine DartPad's storage policies and how we will inform users of them.

I'll land this PR when able and ping you if any changes are needed.

Thanks again!

@Levi-Lesches
Copy link
Author

Hi @parlough, any updates?

@johnpryan
Copy link
Contributor

We added a cookie banner now (#3097), so we should be OK to use local storage.

@Levi-Lesches
Copy link
Author

Levi-Lesches commented Dec 13, 2024

I'm trying to add an integration test that tests:

  • entering text really does save content to the local storage
  • loading the page results in the local storage text being displayed, unless
  • if the page is loaded with a Gist, checks that the Gist is loaded instead of the local storage

This will complicate CI a bit, so I'm updating the README with instructions from the Flutter docs.

@Levi-Lesches
Copy link
Author

Levi-Lesches commented Dec 13, 2024

I ended up going with unit tests instead as I couldn't get the integration tests to work: WidgetTester.enterText(String) calls WidgetTester.showKeyboard(), which requires an EditableText widget somewhere in the subtree, but the current setup with CodeMirror forwards keyboard input directly to the CodeMirror library without using Flutter's TextInput.

The test cases above are still tested, though, so I'm ready for another review.

EDIT ...okay, now I'm ready. I didn't add tests for:

  • Fallback shows when a Gist has no main.dart file, because the flutterdevrelgists account has no such gist
  • Flutter sample ID takes precedence, because I didn't want the tests to break when Flutter docs change

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.

Add support for automatically saving most recent snippet to local storage
3 participants