Skip to content

Conversation

jordanbaron
Copy link

Changed some code to use up-to-date JavaScript and Ethers, as var is widely considered to be obsolete, and Ether's was giving warnings in console since the code was using outdated functions. I also added an optional step to add some styles to the code.

Copy link

@haardikk21 haardikk21 left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great. Just one comment

README.md Outdated
web3.currentProvider,
"ropsten"
);
(async () => await provider.send("eth_requestAccounts", []))();

Choose a reason for hiding this comment

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

Do you mind changing this to use .then() instead of anonymous async-await so as to make it easier for beginners to understand.

Copy link
Author

Choose a reason for hiding this comment

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

I can do that but I could also just change the script tag to be <script type="module"> so top-level await is able to be used, which would make it easier to understand, while also being more up-to-date JavaScript. I'm not good at deciding which is better for newer developers, what do you recommend?

Copy link
Author

Choose a reason for hiding this comment

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

@haardikk21 check my latest commits, I made it use .then :)

Choose a reason for hiding this comment

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

i prefer async/await, but my sole concern here is with the anonymous function. having to do (async () => await ...)() is weird unless you understand it, thats why i suggested .then

thank you!

Choose a reason for hiding this comment

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

please let me know your Discord username so I can give you your role

Copy link
Author

Choose a reason for hiding this comment

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

jords#0763

@haardikk21 haardikk21 merged commit be100f4 into LearnWeb3DAO:master Mar 28, 2022
@jordanbaron jordanbaron mentioned this pull request Mar 29, 2022
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