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

Stuff has been done! #1

Merged
merged 2 commits into from
Jan 8, 2016
Merged

Conversation

Laurentiu-Andronache
Copy link
Contributor

I moved ethereum-connector.js in lib in order to preload the web3 object.

I learned about lib/ preloading from your YouTube video.

I renamed contract.js to ethereum-contract.js because the preloading is done in alphabetical order by filename. I want this file to be loaded second, otherwise it won’t know about the web3 object! I also moved the functions in a file loaded lastly, otherwise it will give an error once about not knowing about the GuessNumberInstance object (even though it will still work fine).

I also ran meteor update on the app.

Tested, now it works flawlessly with both Chrome and Firefox. I don’t know JavaScript btw, I’m learning now. This is my first pull request ever, please tell me what I did wrong!

I moved ethereum-connector.js in lib in order to preload the web3
object.

I learned about lib/ preloading from your YouTube video.

I renamed contract.js to ethereum-contract.js because the preloading is
done in alphabetical order by filename. I want this file to be loaded
second, otherwise it won’t know about the web3 object! I also moved the
functions in a file loaded lastly, otherwise it will give an error once
about not knowing about the GuessNumberInstance object (even though it
will still work fine).

I also ran meteor update on the app.

Tested, now it works flawlessly with both Chrome and Firefox. I don’t
know JavaScript btw, I’m learning now. This is my first pull request
ever, please tell me what I did wrong!
if (typeof web3 == "undefined") {
web3 = new Web3(new Web3.providers.HttpProvider("http://localhost:8545"));
}
else if (!web3.currentProvider)
Copy link
Owner

Choose a reason for hiding this comment

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

I would move that in the same row as the }, so } else if(...) {...}.

and you actually wouldn't need to do anything if web3 already exists, because then you're in mist and the provider is already set.

So please remove line 4-7

@frozeman
Copy link
Owner

frozeman commented Jan 7, 2016

Thanks a lot! and congratulations for your first PR, please fix the issue pointed out in my comments and i will merge.

@Laurentiu-Andronache
Copy link
Contributor Author

I don't know how this works... I've done a commit in my branch with the modifications that you requested. Now... do I have to make the pull request again?

@frozeman
Copy link
Owner

frozeman commented Jan 8, 2016

No if you pushed to your branch, than its already in, e0111fd right?

@Laurentiu-Andronache
Copy link
Contributor Author

Yes, I pushed. e0111fd

frozeman added a commit that referenced this pull request Jan 8, 2016
@frozeman frozeman merged commit 7defe49 into frozeman:master Jan 8, 2016
@frozeman
Copy link
Owner

frozeman commented Jan 8, 2016

Thanks!

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.

2 participants