Page MenuHomePhabricator

SavedWallet balance display + enhancements
AbandonedPublic

Authored by emack on Oct 1 2021, 03:43.

Details

Reviewers
bytesofman
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary
  • 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
Test Plan
  1. npm test passed successfully, which includes unit tests for the newly created formatSavedBalance functions relating to edge cases
  2. Correctly displaying a small XEC balance for saved wallets
  3. Correctly displaying 0 XEC balance for saved wallets
  4. Correctly displaying a large XEC balance for saved wallets
  5. 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')
  6. Correctly displaying 0 balance when viewing a recently imported saved wallet a second time in the Saved Wallets list
  7. Correctly displaying a wallet with 24 characters where it's initially shortened ellipsis style and hovering over it will display the full value
  8. Repeat steps 2-6 for browser extension mode
  9. Test cross browser compatibility in Firefox in standard web mode
  10. Cosmetic check by manually reducing browser width and ensure UI components are positioned accordingly

Diff Detail

Repository
rABC Bitcoin ABC
Branch
local-cashtab-branch
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16866
Build 33571: Build Diffcashtab-tests
Build 33570: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 1 2021, 03:43
emack requested review of this revision.Oct 1 2021, 03:43

Failed tests logs:

====== CashTab Unit Tests:  Configure without a wallet ======
Error: expect(received).toMatchSnapshot()

Snapshot name: `Configure without a wallet 1`

- Snapshot  - 4
+ Received  + 4

@@ -1,7 +1,7 @@
  <div
-   className="sc-kEYyzF jqzglm"
+   className="sc-kkGfuU liSflp"
  >
    <h2>
      <span
        aria-label="copy"
        className="anticon anticon-copy sc-bdVaJa loPqmo"
@@ -67,11 +67,11 @@
          Your seed phrase is the only way to restore your wallet. Write it down. Keep it safe.
        </div>
      </div>
    </div>
    <div
-     className="sc-kkGfuU lbBTMM"
+     className="sc-iAyFgw AbOBK"
    />
    <h2>
      <span
        aria-label="wallet"
        className="anticon anticon-wallet sc-htpNat fLtATP"
@@ -145,11 +145,11 @@
        </svg>
      </span>
       Import Wallet
    </button>
    <div
-     className="sc-kkGfuU lbBTMM"
+     className="sc-iAyFgw AbOBK"
    />
    <h2>
      <span
        aria-label="dollar"
        className="anticon anticon-dollar sc-bwzfXH iTQEyo"
@@ -262,11 +262,11 @@
          </span>
        </span>
      </div>
    </div>
    <div
-     className="sc-kkGfuU lbBTMM"
+     className="sc-iAyFgw AbOBK"
    />
    [
    <a
      className="sc-dxgOiQ jbuVAx"
      href="https://docs.cashtabapp.com/docs/"
    at Object.<anonymous> (/work/web/cashtab/src/components/Configure/__tests__/Configure.test.js:24:18)
    at Object.asyncJestTest (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:106:37)
    at /work/web/cashtab/node_modules/jest-jasmine2/build/queueRunner.js:45:12
    at new Promise (<anonymous>)
    at mapper (/work/web/cashtab/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
    at /work/web/cashtab/node_modules/jest-jasmine2/build/queueRunner.js:75:41
    at processTicksAndRejections (node:internal/process/task_queues:94:5)
====== CashTab Unit Tests:  Configure with a wallet ======
Error: expect(received).toMatchSnapshot()

Snapshot name: `Configure with a wallet 1`

- Snapshot  - 4
+ Received  + 4

@@ -1,7 +1,7 @@
  <div
-   className="sc-kEYyzF jqzglm"
+   className="sc-kkGfuU liSflp"
  >
    <h2>
      <span
        aria-label="copy"
        className="anticon anticon-copy sc-bdVaJa loPqmo"
@@ -105,11 +105,11 @@
          Click to reveal seed phrase
        </div>
      </div>
    </div>
    <div
-     className="sc-kkGfuU lbBTMM"
+     className="sc-iAyFgw AbOBK"
    />
    <h2>
      <span
        aria-label="wallet"
        className="anticon anticon-wallet sc-htpNat fLtATP"
@@ -183,11 +183,11 @@
        </svg>
      </span>
       Import Wallet
    </button>
    <div
-     className="sc-kkGfuU lbBTMM"
+     className="sc-iAyFgw AbOBK"
    />
    <h2>
      <span
        aria-label="dollar"
        className="anticon anticon-dollar sc-bwzfXH iTQEyo"
@@ -300,11 +300,11 @@
          </span>
        </span>
      </div>
    </div>
    <div
-     className="sc-kkGfuU lbBTMM"
+     className="sc-iAyFgw AbOBK"
    />
    [
    <a
      className="sc-dxgOiQ jbuVAx"
      href="https://docs.cashtabapp.com/docs/"
    at Object.<anonymous> (/work/web/cashtab/src/components/Configure/__tests__/Configure.test.js:35:18)
    at Object.asyncJestTest (/work/web/cashtab/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:106:37)
    at /work/web/cashtab/node_modules/jest-jasmine2/build/queueRunner.js:45:12
    at new Promise (<anonymous>)
    at mapper (/work/web/cashtab/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
    at /work/web/cashtab/node_modules/jest-jasmine2/build/queueRunner.js:75:41
    at processTicksAndRejections (node:internal/process/task_queues:94:5)

Each failure log is accessible here:
CashTab Unit Tests: Configure without a wallet
CashTab Unit Tests: Configure with a wallet

Fabien requested changes to this revision.Oct 1 2021, 08:24
Fabien added a subscriber: Fabien.

Thanks for the patch.

There is a broken test from your changes that needs to be fixed. Also is it possible to add a test for the new feature (balances printed as locale strings) ?

This revision now requires changes to proceed.Oct 1 2021, 08:24
bytesofman requested changes to this revision.Oct 1 2021, 20:55

Good progress on this. A few items required before this lands:

  1. Overflow issue on how balances are displayed -- wallets with long names can push buttons and names off the interface. See below:

image.png (272×578 px, 33 KB)

  1. Please create a separate function to format the displayed balance as a locale string. This will help to handle edge cases that could prevent the app from rendering. Input of the function should be sw.state.balances.totalBalance and output should be the localeString of this value. Function should handle undefined or null values -- any error from functionInput.toLocaleString() and the function should return an appropriate marker denoting "balance not available." Balance N/A would be fine.
  1. Please add unit tests for this function. Please confirm npm test is all clear before pushing up an amend to this diff.
  1. General aesthetic improvement; could use some more styling in the new <SWBalance> component, mb a smaller font size or some other distinction from the wallet name column.

Let me know any questions, thanks again!

Summary:
Incorporates review feedback from Fabien and BytesOfMan from 2021-10-01

  • 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 snapshot to fix earlier unit test errors

Test Plan:

  1. npm test passed successfully, which includes unit tests for the newly created formatSavedBalance functions relating to edge cases
  2. Correctly displaying a small XEC balance for saved wallets
  3. Correctly displaying 0 XEC balance for saved wallets
  4. Correctly displaying a large XEC balance for saved wallets
  5. 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')
  6. Correctly displaying 0 balance when viewing a recently imported saved wallet a second time in the Saved Wallets list
  7. Correctly displaying a wallet name with 24 characters where it's initially shortened ellipsis style and hovering above it will display the full value
  8. Repeat steps 2-6 for browser extension mode
  9. Test cross browser compatibility in Firefox in standard web mode
  10. Cosmetic check by manually reducing browser width and ensure UI components are positioned accordingly

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)

emack edited the test plan for this revision. (Show Details)
emack edited the test plan for this revision. (Show Details)
bytesofman requested changes to this revision.Oct 4 2021, 17:01

Hey looking good from the screenshots! Some suggested improvements in line by line comments.

web/cashtab/package.json
160

Why was jest downgraded? If this is not specifically necessary to get the diff to work, the dependency tree should remain unmodified.

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

More robust to use a try...catch loop here. This way, if the function throws any kind of error from Number(swBalance).toLocaleString(), it will be caught and returned as N/A. (compare to approach here, where N/A is only returned in specific case of undefined or null, and any error from .toLocaleString is not handled.

This revision now requires changes to proceed.Oct 4 2021, 17:01
emack edited the summary of this revision. (Show Details)
emack edited the test plan for this revision. (Show Details)

Added try-catch loop to validation.js and reverted jest version in package.json

After further discussion with @bytesofman, due to issues with commit this diff will now be abandoned with a new fresh diff created (D10252).