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
multi: Add wallet balance tooltip and all known wallet balances to translation map #2357
Conversation
I wonder if everything with a tooltip really needs the "i" symbol, or we can just give the text a tooltip that appears on mouse hover? |
I'd say yes. The i symbol help users notice there is a hidden info easily. I've notice in other applications, the tooltip is displayed on the i icon itself and not on surrounding text so I did that in this PR (this is debatable since it's a matter of making it easy to hover on a small icon vs not pushing it in their face). |
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.
Looks pretty good to me. Two small nits though.
Will wait for another review to merge though.
5f99e6b
to
bc9b2e1
Compare
- Added `asset.CustomBalance` struct type for custom balances. Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
bc9b2e1
to
70c4590
Compare
if (bal.contractlocked > 0) addSubBalance(lockedBal(intl.prep(intl.ID_SWAPPING)), bal.contractlocked, intl.prep(intl.ID_LOCKED_SWAPPING_BAL_MSG)) | ||
if (bal.bondlocked > 0) addSubBalance(lockedBal(intl.prep(intl.ID_BONDED)), bal.bondlocked, intl.prep(intl.ID_LOCKED_BOND_BAL_MSG)) | ||
for (const [cat, bal] of sortedBalCats) { |
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.
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.
I also think we should reorganize the display so that the locked sub-balances show as subcategories of the locked total, e.g. roughly
Okay.
We need to display orderLocked as well. Not sure why we weren't already doing that.
Updated in f0b59d0
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.
I also think we should reorganize the display so that the locked sub-balances show as subcategories of the locked total, e.g. roughly
@buck54321, From the comment for these balance fields, It seems they are not included in asset.Balance.Locked
and might be misleading to include them as a subcategory of the locked total, or I'm I missing something?
Lines 111 to 118 in 4a2ba0f
OrderLocked uint64 `json:"orderlocked"` | |
// ContractLocked is the total amount of funds locked in unspent (i.e. | |
// unredeemed / unrefunded) swap contracts. This amount is NOT included in | |
// the db.Balance. | |
ContractLocked uint64 `json:"contractlocked"` | |
// BondLocked is the total amount of funds locked in unspent fidelity bonds. | |
// This amount is NOT included in the db.Balance. | |
BondLocked uint64 `json:"bondlocked"` |
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
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.
Looks and tests well. Just some suggestions for tooltip language.
@@ -53,7 +53,7 @@ func TestFetchBinanceSpread(t *testing.T) { | |||
|
|||
func TestFetchCoinbaseSpread(t *testing.T) { | |||
testSpreader(t, fetchCoinbaseSpread, "btc", "usd") | |||
}x |
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.
weird
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.
yh.
[ID_IMMATURE_TITLE]: 'Immature', | ||
[ID_SWAPPING]: 'Swapping', | ||
[ID_BONDED]: 'Bonded', | ||
[ID_LOCKED_BAL_MSG]: 'Total funds temporarily locked to cover the costs of your bond maintenance, settling matches, and necessary fees', |
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.
settling matches, and necessary fees -> live orders and matches
Also can be locked for other reasons though. For Decred, we add value locked in tickets too.
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.
Updated.
[ID_SWAPPING]: 'Swapping', | ||
[ID_BONDED]: 'Bonded', | ||
[ID_LOCKED_BAL_MSG]: 'Total funds temporarily locked to cover the costs of your bond maintenance, settling matches, and necessary fees', | ||
[ID_IMMATURE_BAL_MSG]: 'Incoming funds awaiting confirmation', |
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.
Still a little off. This is really only coinbase and stakebase outputs that require a large number of confirmations by consensus. Redeemed funds never end up here.
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.
Any other specific message in mind?
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.
Good enough for now. I may follow up with a change later.
[ID_LOCKED_SWAPPING_BAL_MSG]: 'Funds currently locked in settling matches', | ||
[ID_LOCKED_BOND_BAL_MSG]: 'Funds locked in active bonds', | ||
[ID_RESERVES_DEFICIT]: 'Reserves Deficit', | ||
[ID_RESERVES_DEFICIT_MSG]: 'Difference between the available balance in your wallet and the amount that has been reserved for specific purposes', |
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.
The apparent wallet balance shortcoming to maintain bonding level. If this persists, you may need to add funds to stay fully bonded.
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.
I have updated the message with your suggestion in commit 27d2601. But is Reserves Deficit
only for Fidelity Bonds
?
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
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.
LGTM. Another review would be appreciated. Otherwise, I'll merge in a day or two.
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.
Working well on simnet.
Closes #2177
UI changes:
Edit: IMO, reducing the tooltip width and adding border-radius makes the tooltip look slick.