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

Offer to fork non-editable fiddle on Save/Run/Test #120

Merged
merged 2 commits into from Apr 20, 2019
Merged

Conversation

vgrichina
Copy link
Contributor

Fixes #93

@@ -366,6 +366,16 @@ async function saveAll() {

export async function deployAndRun(fiddleName: string, pageName: string = "", contractSuffix: string = "") {
gaEvent(pageName === "test.html" ? "deployAndTest" : "deployAndRun");

// TODO: Remove ugly hack with window
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to copy this todo everywhere? Let's just keep this pattern, since I don't see a good way to solve this any time soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it generally can be solved by moving state management to happen through AppStore, but yeah, easier to hack around for now

@@ -421,6 +422,19 @@ export class App extends React.Component<AppProps, AppState> {
notifyArcAboutFork(fiddle);
}
}

async forkIfNeeded(): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see good way to test it without running whole thing in browser.
Gonna track this here cause initial setup gonna be involved:
#121

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

@janedegtiareva janedegtiareva left a comment

Choose a reason for hiding this comment

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

Please add a test, after that LGTM

@vgrichina vgrichina merged commit 35e1f2f into master Apr 20, 2019
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.

When using someone else fiddle, both "Run" and "Test" should fork first.
2 participants