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

update existing github gist #171

Merged
merged 11 commits into from
Jul 31, 2024
Merged

update existing github gist #171

merged 11 commits into from
Jul 31, 2024

Conversation

magland
Copy link
Collaborator

@magland magland commented Jul 29, 2024

In save project window, user can either save to new gist or update existing gist. When updating the existing Gist, they need to manually enter the Gist URL (https://gist.github.com/<user>/<id>). For this PR we don't keep track of the origin gist URL for the open project (even though it would be convenient) because there are a number of questions about how that would be done. I think that should be a separate PR.

Closes #153

gui/src/app/gists/saveAsGitHubGist.ts Outdated Show resolved Hide resolved
gui/src/app/pages/HomePage/SaveProjectWindow.tsx Outdated Show resolved Hide resolved
@magland
Copy link
Collaborator Author

magland commented Jul 30, 2024

This is going to have a conflict with #158, so after one of these is merged I'll work on resolving it.

@magland
Copy link
Collaborator Author

magland commented Jul 31, 2024

I'm going to need to do increasingly complex conflict resolution for this and #158 once the latest PRs are merged. Is there a reason why these two are not getting merged? I just would like to know when I should incorporate those changes.

@WardBrian @jsoules

@WardBrian
Copy link
Collaborator

I don't have any issue with the functionality of this PR, but I do find the logic to create the GistExplainer div to be a bit hard to follow now

I haven't looked at #158 in a few days. I'm not entirely convinced it's a good functionality to provide (it seems almost certain to me that people will misunderstand it and lose their data), but I'm not so opposed as to prevent it being included

@magland
Copy link
Collaborator Author

magland commented Jul 31, 2024

Okay I'll work on resolving conflicts for this one first, and try to clarify/simplify the GistExplainer

@magland
Copy link
Collaborator Author

magland commented Jul 31, 2024

I've resolved the conflicts here. @WardBrian could you elaborate a bit on where you are confused? Are you talking about this code?

{!exportingToGist && !updatingExistingGist && (
<div>
<Button
onClick={async () => {
serializeAsZip(data).then(([zipBlob, name]) =>
triggerDownload(zipBlob, `SP-${name}.zip`, onClose),
);
}}
>
Save to .zip file
</Button>
&nbsp;
<Button
onClick={() => {
setExportingToGist(true);
}}
>
Save to GitHub Gist
</Button>
<Button
onClick={() => {
setUpdatingExistingGist(true);
}}
>
Update a GitHub Gist
</Button>
</div>
)}
{exportingToGist && (
<GistExportView
fileManifest={fileManifest}
title={data.meta.title}
updateExisting={false}
onClose={onClose}
/>
)}
{updatingExistingGist && (
<GistExportView
fileManifest={fileManifest}
title={data.meta.title}
updateExisting={true}
onClose={onClose}
/>
)}

@WardBrian
Copy link
Collaborator

No, it's really more the code on line 169-on that actually builds up the view -- I understand the desire to share between the uploading and updating case, but it leads to a lot of conditionals and even conditional prose.

I think it would make sense to split them, because a user who is updating an existing gist has already been through the token creation, presumably?

@magland
Copy link
Collaborator Author

magland commented Jul 31, 2024

No, it's really more the code on line 169-on that actually builds up the view -- I understand the desire to share between the uploading and updating case, but it leads to a lot of conditionals and even conditional prose.

I think it would make sense to split them, because a user who is updating an existing gist has already been through the token creation, presumably?

Okay I'll take a crack at that.

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.

I'm okay moving forward with this.

@magland
Copy link
Collaborator Author

magland commented Jul 31, 2024

@WardBrian I made a separate component for updating the gist, and tried to reuse components where it seemed appropriate.

@WardBrian
Copy link
Collaborator

Thanks -- looks great to me!

@WardBrian WardBrian linked an issue Jul 31, 2024 that may be closed by this pull request
@magland magland merged commit 32e17b7 into main Jul 31, 2024
2 checks passed
@WardBrian WardBrian deleted the update-gist branch July 31, 2024 19:15
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.

feature: mechanism for updating gists Gist: Link directly to token creation
3 participants