Page MenuHomePhabricator
Differential D10252

[Cashtab] display balances for saved wallets
ClosedPublic
Actions

Authored by emack on Oct 5 2021, 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
Branch
new-savedwallet-diff
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16884
Build 33607: Build Diffcashtab-tests
Build 33606: arc lint + arc unit

Event Timeline

emack created this revision.Oct 5 2021, 00:57
Owners added a reviewer: Restricted Owners Package.Oct 5 2021, 00:57
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 5 2021, 00:57
emack requested review of this revision.Oct 5 2021, 00:57
emack mentioned this in D10239: SavedWallet balance display + enhancements.Oct 5 2021, 00:59
Harbormaster completed remote builds in B16878: Diff 30298.Oct 5 2021, 01:00
emack added a comment.Oct 5 2021, 01:05

UI screenshots below

1.PNG (270×620 px, 28 KB)

2.PNG (310×598 px, 30 KB)

3.PNG (275×589 px, 27 KB)

4.PNG (505×465 px, 18 KB)

bytesofman requested changes to this revision.Oct 5 2021, 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.Oct 5 2021, 12:31
emack updated this revision to Diff 30316.Oct 5 2021, 13:08

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

Harbormaster completed remote builds in B16884: Diff 30316.Oct 5 2021, 13:11
bytesofman requested changes to this revision.Oct 5 2021, 21:07

Minor nit then ready to land

web/cashtab/src/utils/validation.js
125

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

128

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

This revision now requires changes to proceed.Oct 5 2021, 21:07
emack updated this revision to Diff 30326.Oct 5 2021, 23:23

Minor adjustment to operator to avoid type coercion

Harbormaster completed remote builds in B16893: Diff 30326.Oct 5 2021, 23:26
bytesofman requested changes to this revision.Oct 5 2021, 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.Oct 5 2021, 23:29
emack updated this revision to Diff 30327.Oct 6 2021, 01:12

Updated React idiosyncrasies

Harbormaster completed remote builds in B16894: Diff 30327.Oct 6 2021, 01:16
bytesofman requested changes to this revision.Oct 6 2021, 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.Oct 6 2021, 12:55
emack updated this revision to Diff 30342.Oct 6 2021, 13:11

using currency.cashDecimals for maximumFractionalDigits parameter

Harbormaster completed remote builds in B16908: Diff 30342.Oct 6 2021, 13:13
bytesofman accepted this revision.Oct 6 2021, 13:16
This revision is now accepted and ready to land.Oct 6 2021, 13:16
Closed by commit rABC938e0caca334: [Cashtab] display balances for saved wallets (authored by emack). · Explain WhyOct 6 2021, 13:18
This revision was automatically updated to reflect the committed changes.
emack added a commit: rABC938e0caca334: [Cashtab] display balances for saved wallets.

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
components/
Configure/
63 lines
__tests__/
__snapshots__/
utils/
__tests__/
35 lines
20 lines

Diff 30316

View Options

web/cashtab/src/components/Configure/Configure.js

Loading...
View Options

web/cashtab/src/components/Configure/__tests__/__snapshots__/Configure.test.js.snap

Loading...
View Options

web/cashtab/src/utils/__tests__/validation.test.js

Loading...
View Options

web/cashtab/src/utils/validation.js

Loading...