Closed nathan-lapinski closed 4 years ago
Amazing! Thanks for this contribution @nathan-lapinski!
I'm having some issues setting up my local env (I believe the staging server is down ATM - please confirm @javier-tarazaga) but once that's sorted out I will definitely take a look at this!
@samajammin Thanks!
This looks great @nathan-lapinski! Thanks again for this.
A couple small thoughts. I think it'd be even better to hide some of the text elements if the contract is not deployed. e.g. for the Hello World template: I think it makes sense to hide the "Message:" & "Block number:" texts until the contract is deployed. Perhaps we could add a "Welcome!" message in their place until the contract is deployed.
Same with CryptoPizza:
I think it'd be best to also hide the navigation tab & "Create new CryptoPizza" until the text is deployed.
@samajammin Thanks for the reviews and feedback!! I will implement the UI changes that you have suggested, and will make the same changes to the Coin template as well, and add them to this PR. I should be able to complete it this weekend π
Sorry, ended up being a little swamped this weekend. I'll try to have these changes up in the next couple of days π
@samajammin @RONIZAM80 I think this one is ready for another round of review π Sorry for the delay.
I've updated the Coin template to display a welcome message until a contract is deployed. I've also refactored all three template UI's, per @samajammin 's comments above. Now, only a welcome message will be displayed until a contract is deployed:
I've also split the app.html
files into two sections:
<div id="welcome-container">
which displays until a contract is deployed, and
<div id="main-container" class="hidden">
, which is initially hidden
, and displays once a contract has been deployed.
Please let me know if there are any additional changes or updates that you'd like to see in this PR. Thanks in advance for the reviews!
@javier-tarazaga please give this a final review. I believe it's ready to ship π
This looks awesome @nathan-lapinski !! Thank you so much for this :)
Let me give it a quick test in staging before promoting to prod!
Thanks once again!
Thanks @javier-tarazaga and @samajammin !!
@javier-tarazaga the changes in staging LGTM - can we push to prod?
@nathan-lapinski so finally the code is in prod! :)
Description of the Change
This change updates some of the template
app.js
files to check ifContract.address
is set before displaying any UI input elements. It also includes some html/css changes to hide input elements by default, and display a welcome message instead.So far, I've only added updates to Hello World and Crypto Pizza. If the community is ok with the approach of this PR, I'd be happy to update the Coin template as well.
Alternate Designs
I'm not sure if checking
Contract.address
is the best way to determine whether or not the contract has been deployed. I was (and still am) looking to see if there's any kind of event or Promise I can listen to and be notified when the contract is deployed. If anyone knows of such a way, please let me know.However, checking
Contract.address
appears to work, so I thought I'd submit here as a first attempt.Benefits
Users will no longer see input fields that they can't yet interact with before deploying a contract. For example, in Hello World, entering a message and submitting would cause the app to crash (since
this.instance
was not yet fully defined)Possible Drawbacks
It assumes that checking
Contract.address
is a reliable way of determining if the contract has been deployed. That could potentially make things brittle.Verification Process
node scripts/generate-templates.js
npm start
7 . I entered "test message" in the "Update message" input, and clicked send
Crypto Pizzas
node scripts/generate-templates.js
npm start
I clicked the deploy button
The welcome message went away, and the UI inputs were rendered
I entered "olives" into the "enter name..." input, and clicked "create". It was successful
How did you verify that all changed functionality works as expected? Manually built the app locally and dev tested it. Same steps as above
How did you verify that the change has not introduced any regressions? Other than manual testing of the app, I haven't really. Are the template files currently covered by tests? I ran into some issues trying to run tests from the project root, as well as the /editor root.
Error: Cannot find module '../src/components/solc/dist/soljson-v0.5.10.js'