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

save project as gist #114

Merged
merged 3 commits into from
Jul 3, 2024
Merged

save project as gist #114

merged 3 commits into from
Jul 3, 2024

Conversation

magland
Copy link
Collaborator

@magland magland commented Jun 29, 2024

Replaces #101 because needed to rebase on the prettier formatted main branch.

Renamed import / export to "Load project" / "Save project". Try it out and see how it feels.

Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Finally got around to trying this out - looking pretty good to me! A few comments mostly on the prose of the instructions (we may also want to include a screen shot or gif?)

gui/src/app/pages/HomePage/SaveProjectWindow.tsx Outdated Show resolved Hide resolved
gui/src/app/pages/HomePage/SaveProjectWindow.tsx Outdated Show resolved Hide resolved
Comment on lines 144 to 146
For security reasons, your token will not be saved in this
application,  so you may want to store it securely in a text file
for future use.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we had decided local storage was probably fine for this?

Copy link
Collaborator Author

@magland magland Jul 2, 2024

Choose a reason for hiding this comment

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

If it's okay, let's make that a future feature. I want to think through security implications more carefully and how to present this to the user. I think saving in browser should be opt-in only.

gui/src/app/pages/HomePage/saveAsGitHubGist.ts Outdated Show resolved Hide resolved
o: { defaultDescription: string; personalAccessToken: string },
) => {
const { defaultDescription, personalAccessToken } = o;
const description = prompt(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh also -- this prompt is redundant with the title box already on the save page, right?

I guess it's nice as an "are you sure"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was intentional. When saving to a gist it's important to specify an appropriate title/description, and I often find myself forgetting to do this, even when the option is there in the table, and so this is a very helpful opportunity.

@magland
Copy link
Collaborator Author

magland commented Jul 2, 2024

Unless you want to discuss the storing PAT to browser further, this might be ready to go.

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

Couple cleanup suggestions for the main code file, but otherwise I think this is in good shape.

gui/src/app/gists/saveAsGitHubGist.ts Outdated Show resolved Hide resolved
gui/src/app/gists/saveAsGitHubGist.ts Outdated Show resolved Hide resolved
for (const key in files) {
// gists do not support empty files or whitespace-only files
if (files[key].trim() === "") {
console.warn(`File ${key} is empty or whitespace-only. Not saving.`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to think in the future about whether we need to warn this, since in a data.py/r analysis.py/r world we will expect the presence of empty files. (Fine for now.)

@@ -0,0 +1,213 @@
import { FunctionComponent, useCallback, useContext, useState } from "react";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noting that this file is getting a little chunky--might consider splitting into separate files for the major components at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed

@jsoules
Copy link
Collaborator

jsoules commented Jul 3, 2024

Changes look good. I think we're ready to merge if @WardBrian is okay with the language.

@magland magland merged commit 1d4f3a8 into main Jul 3, 2024
2 checks passed
@WardBrian WardBrian deleted the export-to-gist-4 branch July 8, 2024 19:58
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