Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

[WIP] Export to Codesandbox #324

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

[WIP] Export to Codesandbox #324

wants to merge 11 commits into from

Conversation

AWolf81
Copy link
Collaborator

@AWolf81 AWolf81 commented Nov 4, 2018

Related Issue:
#323

Summary:
Exporting to Codesandbox working in branch export-to-codesandbox. Just the mentioned issue with the image not fixed yet in Codesandbox. I have to check what's wrong with it.

I'm creating this because I'd like to get a first feedback if this is going into the right direction. If it's OK then I'll start to refactor the business logic from the component into export-codesandbox.saga.

Save button styling at the end of the dialog is a bit messed. I have to check why this happened.

I think the export flow is OK like it is in the branch. The Codesand token is saved in app-settings.reducer so it's available for every project.

TODOs

  • Improve styling of save button & export section (more padding)
  • Check if we should hide the token and only display a check mark that it's available.
  • Make a copy sandbox url to clipboard button for easier sharing. (Maybe optional, not sure if it's needed) - not needed as there is an open button then the user can use the share options inside Codesandbox
  • Refactor to export-codesandbox.saga
  • Add unit tests
  • Check Codesandbox logo issue in the exported sandbox
  • Add logout test
  • Check flow errors in export.saga & test - multiple $FlowFixMe added.
  • Add try/catch blocks around the spawn of codesandbox - shouldn't happend but we need to show an error message like Export failed. Please check browser console for more details. & also finish exporting.

Screenshots/GIFs:
grafik

<ExportToCodesandboxWrapper>
<FormField size="small" label="Export to Codesandbox.io">
<InfoText>
Goto Codesandbox website by clicking the link and copy the token.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Goto Codesandbox website by clicking the link and copy the token.
Go to the Codesandbox website by clicking the link and copy the token.

@codecov
Copy link

codecov bot commented Dec 10, 2018

Codecov Report

Merging #324 into master will increase coverage by 0.55%.
The diff coverage is 29.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #324      +/-   ##
==========================================
+ Coverage   20.91%   21.47%   +0.55%     
==========================================
  Files         246      255       +9     
  Lines        3896     4094     +198     
  Branches      389      413      +24     
==========================================
+ Hits          815      879      +64     
- Misses       2802     2922     +120     
- Partials      279      293      +14
Impacted Files Coverage Δ
src/components/Modal/Modal.js 0% <ø> (ø) ⬆️
src/config/app.js 100% <ø> (ø) ⬆️
src/store/index.js 0% <0%> (ø) ⬆️
src/components/ModalHeader/ModalHeader.js 0% <0%> (ø) ⬆️
src/components/DisabledText/index.js 0% <0%> (ø)
src/components/TokenInput/TokenInput.js 0% <0%> (ø)
src/components/ExportToCodesandbox/index.js 0% <0%> (ø)
src/sagas/index.js 0% <0%> (ø) ⬆️
src/components/FormField/FormField.js 0% <0%> (ø) ⬆️
src/components/DisabledText/DisabledText.js 0% <0%> (ø)
... and 22 more

@melanieseltzer

This comment has been minimized.

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Dec 10, 2018

OK, I think this is ready for a review.
I have two smaller things open but it would be great to get a feedback to it.

The UI is not perfect yet. If you're having any ideas what should be improved please let me know.
What do you think? Is it OK to move the save button to the header of the modal (see screenshot above)?
I've moved it to save some space in the modal content - otherwise it wasn't fitting. Sure adding a scroll would help but I don't like to have the save button of screen.
Or maybe we could add the save bottom to a bottom bar that's permanently visible and use a scrollbar.

@melanieseltzer where do you get that message? Is this happening after triggering export action?

@melanieseltzer

This comment has been minimized.

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Dec 10, 2018

Seems like codesandbox cli is missing. Have you installed it with a yarn call?
If it's installed and still happening I think that could be a path issue.

package.json Outdated
@@ -48,6 +48,7 @@
]
},
"dependencies": {
"codesandbox": "^1.3.5",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I've missed to pin the version. I'll change that in my next commit.

@melanieseltzer
Copy link
Collaborator

@AWolf81 🤦‍♀️ That was it, whoops.

I'm not too familiar with Codesandbox and the CLI, do you know why I'm getting an infinite loop of tokens? Every time it finds error with the token and prompts me to get a new one. Am I doing something wrong?

kapture 2018-12-09 at 17 07 02

{
// Would be good to not go to codesandbox as the user already entered the token
// we need to open codesandbox because otherwise there won't be a question for the token
// --> current CLI api doesn't support what we need here. A command line arg for the token would help.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to double check this with a fresh state. I think this behaviour changed during my work on it.
I think it's working as expected and not opening a page to codesandbox but I have to check this.

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Dec 10, 2018

@melanieseltzer no problem. :-)

That's weird. Maybe sendCommandToProcess is not properly working as I'm seeing many characters from the token in the terminal log. I've had a similar behaviour before I've added the currentStep counter - maybe this is not working as expected.

The Codesandbox shouldn't loop. The normal CLI flow is like following:

  • After calling codesandbox .
  • It opens a browser window to display the token
  • Then the CLI asks for the token (copy&paste) to the terminal
  • Finally it asks to proceed with deployment y/n

The token is stored at the CLI location that's why I've added a logout button which will call codesandbox logout.

@AWolf81
Copy link
Collaborator Author

AWolf81 commented Jan 17, 2019

@melanieseltzer I had the same behaviour like you. Seems like a node version issue with codesandbox cli.
What node version do you use? If it's >10 there is an issue as the CLI doesn't open the browser window & it's stuck before opening the browser. v8 is working.

Issue on Codesandbox Cli already tracking it. Seems like it is related to opn package - I'll have a look into that package.

@superhawk610
Copy link
Collaborator

My first shot at getting this running resulted in getting stuck with a loading spinner on the Export to Codesandbox button and no sandbox created on their end. Error message is spawn codesandbox ENOENT, running on Ubuntu 18.04. I'll update this comment as I figure things out.

@superhawk610
Copy link
Collaborator

If the looping issue is due to codesandbox cli, then we can potentially fork our own version and just set a fallback if something goes wrong, maybe log the intended URL to the console and have the user ctrl+click on it instead. I'll dig into this further and see if I can replicate it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants