Page MenuHomePhabricator

[Cashtab] display balances for saved wallets
ClosedPublic

Authored by emack on Tue, Oct 5, 00:57.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC938e0caca334: [Cashtab] display balances for saved wallets
Summary

Starting this new diff after encountering commit issues with D10239 which will now be abandoned.

  • Created new function 'formatSavedBalance' in utils/validation.js to take a savedwallet balance as input and return a formated locale string so large XEC balances are legible
  • Imported the newly created locale string function into Configure/Configure.js and utilised as part of conditional rendering to mitigate edge cases that may prevent the app from rendering.
  • Added unit tests in validation.test.js to test edge case inputs for SWBalances, including undefined/null balances, zero and max supply trillion balances
  • Fixed overflow issue with long wallet names pushing buttons off the UI by adjusting overflow:hover css for both <SWName> and <SWBalance>
  • Added additional styling in Configuration.js's <SWBalance> component to enhance aesthetics and distinguish SWBalance values from SWName
  • Updated jest snapshot for Configure.js
Test Plan
  • npm test passing successfully, which includes unit tests for the newly created formatSavedBalance functions relating to edge cases
  • Correctly displaying a small XEC balance for saved wallets
  • Correctly displaying 0 XEC balance for saved wallets
  • Correctly displaying a large XEC balance for saved wallets
  • Correctly displaying N/A balance for a newly imported saved wallet (this will test undefined or null inputs from 'sw.state.balances' and 'sw.state.balances.totalBalance')
  • Correctly displaying 0 balance when viewing a recently imported saved wallet a second time in the Saved Wallets list
  • Correctly displaying a wallet with 24 characters where it's initially shortened ellipsis style and hovering over it will display the full value
  • Repeat steps 2-6 for browser extension mode
  • Test cross browser compatibility in Firefox in standard web mode
  • Cosmetic check by manually reducing browser width and ensure UI components are positioned accordingly

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Tue, Oct 5, 00:57
emack requested review of this revision.Tue, Oct 5, 00:57
bytesofman requested changes to this revision.Tue, Oct 5, 12:31

Looks good. One more request:

For the formatSavedBalance function, do not default the locale to en-US. Some Cashtab users e.g. in Europe are getting standard toLocaleString formats in other places of the app that are not hardcoded to US.

One way to do this while keeping the specific unit tests is to have formatSavedBalance accept an optional parameter for US balances, then specify that parameter only in the unit tests. Another approach would be to revise the unit tests to only check for type instead of exact formatting expectation.

This revision now requires changes to proceed.Tue, Oct 5, 12:31

Added optional locale parameter for formatSavedBalance function and adjusted unit tests.

bytesofman requested changes to this revision.Tue, Oct 5, 21:07

Minor nit then ready to land

web/cashtab/src/utils/validation.js
125 ↗(On Diff #30316)

Using === is a standard in the repo, want to avoid type coercion.

128 ↗(On Diff #30316)

Using === is a standard in the repo, want to avoid type coercion.

This revision now requires changes to proceed.Tue, Oct 5, 21:07

Minor adjustment to operator to avoid type coercion

bytesofman requested changes to this revision.Tue, Oct 5, 23:29
bytesofman added inline comments.
web/cashtab/src/components/Configure/Configure.js
588 ↗(On Diff #30326)

React idiosyncrasy: replace class= with className=, i.e. className="overflow"

591 ↗(On Diff #30326)

Same here

React idiosyncrasy: replace class= with className=, i.e. className="overflow"

This revision now requires changes to proceed.Tue, Oct 5, 23:29

Updated React idiosyncrasies

bytesofman requested changes to this revision.Wed, Oct 6, 12:55
bytesofman added inline comments.
web/cashtab/src/utils/validation.js
130 ↗(On Diff #30327)

This is correct and will probably never change. However, Cashtab has been configured for easy decimal moves to support the redenomination from 8 decimal places to 2. Want to make sure that this variable is used everywhere instead of hard-coded '2'.

import { currency } from '@components/Common/Ticker.js';
...
maximumFractionDigits: currency.cashDecimals,
134 ↗(On Diff #30327)

Same - replace "2" with currency.cashDecimals

This revision now requires changes to proceed.Wed, Oct 6, 12:55

using currency.cashDecimals for maximumFractionalDigits parameter

This revision is now accepted and ready to land.Wed, Oct 6, 13:16
This revision was automatically updated to reflect the committed changes.