Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
project save/load to browser #158
base: main
Are you sure you want to change the base?
project save/load to browser #158
Changes from 2 commits
6819225
fa97940
cc62b22
b415613
d0eb52a
e8fe625
041176a
67b7ba5
8188815
1a098fb
6ba92cf
ffb6874
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there is a race condition here if you have two tabs open: saving (or deleting) a project in one will not update the other.
I think this is probably okay, it's not really an intended operational mode or likely to come up, but I wanted to point it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but this is not going to be a problem since retrieving from browser storage is fast enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a matter of save/retrieve speed--it's if you have the window open already, there isn't anything triggering an update. (At least that's how it looked when I was playing around with it.)
Again, I think the steps to trigger are kind of far-fetched, so I'm not worried, but just want to note everything I see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see! Didn't read carefully enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like it would also be incredibly helpful to store a timestamp (we could put this in
meta.json
in general). That way we could display it here if we supported multiple local saves by the same name, or display it in the "are you sure you want to overwrite" dialog so people know when the to-be-overwritten project comes fromThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it would be helpful. But I'm wary about including it in meta.json as there's nothing that is enforcing the accuracy of that information if someone edits the project outside of SP. And for Gists, this information would be available independent of the content of meta.json.
Let me put the timestamp in the browser storage record and we'll see how that looks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea (modulo all the usual complications about timestamps/time zones) but might be for a different PR? I'd want to see the other forms of persistence be aware of it also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Didn't see your message Jeff until just now.)
I added timestamps for the saved browser projects, but it's not in meta.json.
The timestamps are now shown (in the form of "time-ago-strings", like "3 min ago").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we have this elsewhere in this file too--what's the intended use case? As far as I can see, it's just hard-coded to a no-op?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the name to
onCancel
which should be clearer. This is called if user clicks the CANCEL button.