-
Notifications
You must be signed in to change notification settings - Fork 395
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
Add post mint status message for dapp #178
Conversation
Hi @basememara thank you very much for this contribution (and for showing the result on video... hat helps a lot reviewing the PR). I like the idea but my plan was to implement a slightly different feedback. I want to review the code in order to have the full picture of what you did and then I will give you some feedback about this. I hope you will be willing to make some changes and/or maybe work on this together. |
Yes absolutely! A couple things I didn't like what I did here is:
Thank you for putting all this together and taking the time to review this! |
@basememara Well done for getting this moving. Just thinking if Number of tokens is more than 1 it might be hard to get all the token ids? |
Ah ya there's the I couldn't find the token ID anywhere in the transaction object which would be ideal. Or maybe there's some information we can carry thru the purchase process, or some API of ethers.js instead of the contract API. |
The last item could be a good way to go when multiple minted ye ;) |
great idea. thank you for sharing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great changes, than you so much for sharing them. It works like a charm :)
Just my 0.02:
#1.
Etherscan should not be hardcoded
So instead of:
<li><a href={this.generateTokenUrl()} target="_blank">Etherscan</a></li>
We should have:
<li><a href={this.generateTokenUrl()} target="_blank">{this.state.networkConfig.blockExplorer.name}</a></li>
#2.
Also, don’t think it makes sense to have a link to Metamask and Rainbow at this stage of the user experience
Do you guys think there's a way to add a confirmation/error message feedback when the transaction is completed on the minting dapp? |
Hi @basememara, I hope this works and GH will pick the correct commit authors. We will do some tests before merging this. |
Thanks for your kind thoughts and consideration. Much appreciated for your original massive contribution in the first place and taking this further. Whatever happens with GH is ok and am grateful what little contributions I've made helped. |
On mint, disables action buttons and renders a success message with details on how to track and view the NFT. Confirmation button allows to refresh dapp to mint another NFT. Further discussion in issue #11.
Below is a video of the flow:
Screen.Recording.2022-03-28.at.3.00.13.PM.mov