-
Notifications
You must be signed in to change notification settings - Fork 1
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?
Conversation
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.
A couple minor tweaks and questions.
I agree that we should give some warning that this mode of saving should be considered potentially volatile, since a cleared cache etc. would remove it.
type SaveToBrowserViewProps = { | ||
fileManifest: Partial<FileRegistry>; | ||
title: string; | ||
onClose: () => void; |
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.
useEffect(() => { | ||
const bpi = new BrowserProjectsInterface(); | ||
bpi.listProjects().then((titles) => { | ||
setBrowserProjectTitles(titles); | ||
}); | ||
}, []); |
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 :)
Is this okay to merge @WardBrian @jsoules ? |
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.
Two things that stood out to me when trying it just now
component="button" | ||
underline="none" | ||
> | ||
{title} |
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 from
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 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.
Co-authored-by: Brian Ward <[email protected]>
Add browser project functions and time ago string utility
This one is on hold for now because there are some questions about whether we want to have this functionality. Will revisit later. |
Save projects to browser (uses IndexedDB)
Click "Save Project" button, then "Save to Browser"
It will be saved by the title of the project. If there already exists such a project, user will be prompted to replace.
Click "Load Project" button. Below the upload box the browser projects are listed. You can also click the delete icon to delete browser projects.