Add fork button #278
Add fork button #278
Conversation
…ly sketch. Previously, a view-only sketch would be rendered with the same thumbnail and language as the user's most recent program.
Upon clicking the fork button, a modal appears to confirm the fork. If the fork button is clicked, the fork is initiated. Upon completion, a message appears alerting the user that the fork has completed.
|
I'm unsure about the styling of the fork button (what color?) and if the wording in the modal should be changed. |
|
I personally like the look of it as is right now, actually, but that's mostly because it's reminiscent of the GitHub fork look. Something that is a bit of a concern is that I feel like the icon is pretty unfamiliar for new coders and would warrant an explicit label, but then it wouldn't look as clean :sigh:
|
Yeah, I agree.
A nice thing is that having an explicit label would look consistent with the Save button in the editor. However, the function of the button is pretty clear once you click on it and read the description. I don't feel too strongly either way. |
Yeah that's actually a huge plus. I'm leaning more and more towards giving it a label and leaving it at that. In terms of the manual/automatic redirect business, we still have to figure out a cleaner way of doing that -- might be a separate issue entirely since it touches on the editor and also the deleteModal. |
|
So after further discussion, I think what's on live is from a feature-perspective reasonable enough. Definitely not in the right state of mind to do a code review, but @krashanoff take a look when you can (or tag someone in). |
|
I'll take code review on this one! I agree that things are looking good 👍 Should have a review out by the end of the week. |
|
@malsf21 Thanks for the review! I fixed the typos. |
There was a problem hiding this comment.
Hey Jamie! Sorry for the delay on getting this review out. Everything looks solid to me, but I did encounter a bug. I was just testing around with it and everything was looking good until I performed the following sequence of actions:
- Navigate to a nonexistent sketch in the viewonly editor. For example:
/p/bogus. - Navigate to a sketch that exists in the viewonly editor. When I encountered this bug, I was using my own sketch.
- Fork the sketch
- Click OK to redirect
- Crashes as it thinks we are using a Redirect outside of a router.
I can't replicate this on the netlify deploy, but can consistently perform it locally. Anyone else see this issue?
I just tried out the sequence of steps locally (using one of my own sketches as well) and weirdly I was unable to reproduce the bug. Tagging @malsf21 to see if he can? |
There was a problem hiding this comment.
Just redid the same sequence on my other machine and things were okay. I may have been using a 2-3 weeks old versions of our deps on my desktop compared to the latest versions which I used on my laptop which led to the error (whoops!). While I won't lie that it is concerning that this doesn't work on an older version of our deps, if it works on our current deps I have no problem with merging this now. May warrant further investigation on my end. Thanks a ton Jamie!
|
Ok, great! I resolved the merge conflicts and made sure the "share" button is hidden in view-only mode, so this feature should be good to go now! |
|
Phew! Glad we caught that one. I'll go ahead and merge this now that things are squared away. I'll be marking this as a minor release this week to commemorate the addition of the share and fork buttons. Thanks again Jamie! 👍 |
Adds the forking feature to a view-only sketch if the user is logged in.
After pressing the fork button, a modal appears asking the user to confirm the fork.
Once the sketch is successfully forked, the user is prompted to visit their sketches page or close the modal.
If opened again, the modal remains at the final prompt.