Closed ByteAtATime closed 1 month ago
Hey @ByteAtATime thanks for this, it looks cool!
A few open questions/comments before reviewing the code:
The icon might be too big. e.g.
Is /
the best character to represent the change to ETH? Are we "toggling"? if so, we might want a different icon to to toggle back to wei.
Let's see what others think too!
Thanks again sir!
- The icon might be too big. e.g.
Honestly, it probably is, just wasn't sure what the best practice is considering this is already btn-xs
from daisyUI. I'm a little hesitant to override styles manually, but I think that might be the best way?
- Is
/
the best character to represent the change to ETH? Are we "toggling"? if so, we might want a different icon to to toggle back to wei.
Hmm, I'm not completely sure where I got that from. I suppose it's to "divide" (by 1e18), but then I guess we would have to replace it with "*" for "multiply by 1e18". I agree with your "toggle", would the "ArrowsRightLeft" icon work better?
Thanks @ByteAtATime !! looking great ! Added a couple of comments and just pushed a small commit at https://github.com/scaffold-eth/scaffold-eth-2/pull/853/commits/37df0ec817d1a5ab53cc44353cd0597f269a0abb since there was small bug at TxReceipt
component more context here
In DisplayContractVarible part, the nested items inside struct and array are highlighted more as compared to normal variable :
if its get too tedious to handle this we could also have another PR which focuses on styling and maybe also have accordian to display complex variables
Now we're showing the int value, with the "Format as ether" tooltip. Some might think we're converting it to ETH (Ξ symbol on result helps with confusion).
Calculating the symbol of the ERC20 to show it on the tooltip + result may be a good solution, but probably would make the code messy.
Well, the thing is that the contract is not passed into the function, and even if it were we would also need to load the decimals
and everything.
If so, maybe changing the tooltip to "Divide by 1e18" helps a bit?
I think this might be the best approach, I will add a commit. If anyone else has any ideas, I'd be happy to hear them!
Or maybe setting the default result to the old one? And letting user multiply.
The problem with this approach is that it might confuse the user that they're receiving a decimal for a uint256
function. (Not sure about this, but I think it might be a problem for new users).
Just thinking out loud! Maybe it's just me that felt it a bit confusing
Nope, it's definitely a problem we need to fix! Never thought of testing it with other contracts haha, great testing!
, with the "Format as ether" tooltip. Some might think we're converting it to ETH (Ξ symbol on result helps with confusion).
Wooah lol didn't thought it but makes sense !!
Calculating the symbol of the ERC20 to show it on the tooltip + result may be a good solution, but probably would make the code messy. If so, maybe changing the tooltip to "Divide by 1e18" helps a bit?
Umm ERC20 symbol will be adding complexity and I think its not worth it that much (sound a great feature for Abi.Ninja though). Like "Divide by 1e18" idea! I think for now it is best approach.
Or maybe setting the default result to the old one? And letting user multiply.
The problem with this approach is that it might confuse the user that they're receiving a decimal for a uint256 function. (Not sure about this, but I think it might be a problem for new users).
Yeah I agree with this also keeping things a bit raw can make it more intuitive for developer too
Tysm @ByteAtATime !! I think we are almost there just added last couple of comments but its looking great !
Noticed that
overflow-auto
sometimes tooltip looks clipped. Not sure hot to fix it easily though. We can add paddings to result but for large tx result with scrolling we still can meet thisOh my god... I'd forgotten about this PR!
Thanks @technophile-04 for all the commits and fixes!
What are the main issues that need to be fixed? I see that the <NumberDisplay />
has weird issues with the button, is that the only issue that we have right now?
Description
This PR improves the formatting of the transaction result in the debug page, especially for numbers, tuples, and structs.
Here is a table comparing the changes (click on images to enlarge):
arr
field is nicely indented.The contract used in these examples can be found here.
Additional Information
I would consider this to be a "straightforward" change, so have not filed an issue.
Your ENS/address:
byteatatime.eth