-
Notifications
You must be signed in to change notification settings - Fork 212
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
Agent: use formatTokenAmount()
and add rounding tooltip
#1148
Agent: use formatTokenAmount()
and add rounding tooltip
#1148
Conversation
Voting: update VoteText's address rendering (aragon#1146)
@ECWireless Thank you for this!
I think we might want to reproduce all the changes made in #1121, because the codebase of the two apps is really similar:
Let me know if you need anything! |
- Adds formatTokenAmount() and BN.js values everywhere applicable except in useDownloadData.js.
@bpierre @Evalir sorry for the long delay in adding more to this; just finished moving from one side of the US to the other... But I've made a few more additions to the Agent App! This primarily includes just adding formatTokenAmount() from aragon/ui, as well as BN.js, into any appropriate files. I believe the only other file that needs updated is useDownloadData.js, although I'm having trouble testing out changes since Chrome is blocking all downloads from iFrames. Not sure if there is an easy way around this... But let me know what you think! I'll keep messing around with useDownloadData.js in the meantime. |
Ah, this is a good catch. We may need to update our |
On the above, this is fixed with: aragon/client#1436. |
formatTokenAmount()
and add rounding tooltip
- formatTokenAmount() from utils - formatDecimals() from utils - ROUNDING_AMOUNT from utils - LOCALE_US_FORMAT from locales - round() from math-utils
@ECWireless I'm wondering if we could double check and align the I know for |
…nce and Agent app
@sohkai Ah, got it! I just copied over BalanceToken.js and Balances.js and everything works fine. It looks like Finance's Transfers.js and Agent's Transactions.js can't simply be copied between each other though, so I'll go through it again and try to align it a bit more. |
@sohkai I believe Agent's Transactions.js and Finance's Transfers.js are pretty well aligned now. I double-checked it, and all I found was a repeated 'symbol' (because of adding formatTokenAmount() from the UI). |
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.
Looking super good @ECWireless! We're almost there 🏁!
I noticed a few last things that I wanted to check!
@@ -257,11 +257,9 @@ const Transactions = React.memo(function Transactions({ | |||
|
|||
const { symbol, decimals } = tokenDetails[toChecksumAddress(token)] | |||
const formattedAmount = formatTokenAmount( | |||
amount, | |||
isIncoming, | |||
isIncoming ? amount : -Math.abs(amount), |
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.
Oh interesting, do we only have a BN object for amount
at this moment? Otherwise, we should use amount.neg()
.
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.
Ah, I see why now.
I've updated the frontend reducers to push BNs into these transfer objects so we can transparently consume them as BNs elsewhere :).
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.
🙌 👌 🙏 ✅ Thanks @ECWireless, this was a great PR! Very excited to get this out!
I've just pushed a few commits to align it again with Finance and update the state to be BN-centric :).
This first commit simply re-adds the reverts @bpierre made in #1121
Fixes #1157 #756 #1065 #1128.
This PR is still a work in progress. @bpierre are there any other issues I could tack onto this PR?
To do: