search

flatironinstitute / stan-playground

Run Stan models in the browser
Apache License 2.0
36 stars 1 forks source link

update existing github gist #171

Closed magland closed 4 months ago

magland commented 4 months ago

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

magland commented 4 months ago

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

magland commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

magland commented 4 months ago

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

https://github.com/flatironinstitute/stan-playground/blob/8476c9652453341228aa2b684d8843d51c07ec0f/gui/src/app/pages/HomePage/SaveProjectWindow.tsx#L51-L94

WardBrian commented 4 months ago

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 commented 4 months ago

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.

magland commented 4 months ago

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

WardBrian commented 4 months ago

Thanks -- looks great to me!