Page MenuHomePhabricator

[Cashtab] Added toggle hide balance switch
ClosedPublic

Authored by kieran709 on Oct 26 2022, 20:06.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC3f04b8feaa69: [Cashtab] Added toggle hide balance switch
Summary

Related to T2729. Added toggle switch in BalanceHeader component which controls toggleHideBalance setting in cashtabSettings object. BalanceHeaderFiat has also been changed to show or hide user balance in fiat using the same switch. Validation tests have been altered to account for the new setting. CashtabSettings is now passed down through the BalanceHeader component.

Test Plan

cd web/cashtab && npm start
from the home screen, toggle the switch
observe that xec and fiat balances are hidden
navigate to the send screen
observe that the balances are still hidden
from the send screen, toggle the switch
observe that the balances are now showing

check that setting persists across each component that contains the BalanceHeader

Diff Detail

Repository
rABC Bitcoin ABC
Branch
toggle-show-hide-balance-switch
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 20777
Build 41216: Build Diffcashtab-tests
Build 41215: arc lint + arc unit

Event Timeline

Nice switch design and functionality is great.

  1. The switch should not change position when it changes value. Try to place it in the top right corner of the component.
  2. Add a balance header to the Receive page
web/cashtab/src/components/Common/BalanceHeader.js
18 ↗(On Diff #36013)

There's no need to have this be an option in cashtab settings. The toggle switch itself is the setting.

This revision now requires changes to proceed.Oct 27 2022, 16:45
web/cashtab/src/components/Common/BalanceHeader.js
18 ↗(On Diff #36013)

The initial task did request that is be a setting in cashtabSettings.

From T2729
"... the switch should still be toggling a settings param, so that the app remembers the setting across screens."

That said I can change it up if wanted. Making it a setting does seem like a clean way to implement it.

web/cashtab/src/components/Common/BalanceHeader.js
18 ↗(On Diff #36013)

Sorry I thought I deleted this comment before I submitted the request for changes. Please ignore it.

Your implementation is correct and the switch should change a setting.

Responding to review feedback. I decided to make the switch a separate component. The switch in the top right of the banner looked off, so I am trying it on the same line as the wallet name and wallet rename button. The balance header has now been added to the receive.js page.

Switch positioning is much better.

The current implementation (rendering or not rendering the balance based on the switch) may be the best way to do it. However, I still really don't like content jumping anywhere in the app.

Ideally, hitting the switch will not cause any content jumping.

Best case scenario: a hidden balance has a neat effect, like spoilers in Telegram:

image.png (86×653 px, 14 KB)

Next best: render dashes or some other same-sized component that illustrates the balance would be there but is hidden

Implement this and we'll compare if it's better than the current full-hide version with content jumping.

web/cashtab/src/components/Common/HideBalanceSwitch.js
9 ↗(On Diff #36068)

Because the parent components of this all already use WalletContext, changeCashtabSettings should be passed down as a prop and not initialized in this component

This revision now requires changes to proceed.Oct 28 2022, 22:09

Per review feedback: a blur effect is placed on the balance on toggle to avoid content jumping, changeCashtabSettings is passed to HideBalanceSwitch as a prop instead of initialized within the component.

Feature and styling look great. However, it is annoying that the UI locks when the switch is hit.

On reviewing the changeCashtabSettings function, this UI lock is implemented specifically to prevent incorrect balances being displayed when the fiat currency is updated. This makes sense. However, this was the first setting to be implemented. Other settings changes do not have this restriction.

Please modify the changeCashtabSettings function so that the UI is not locked when this setting is changed.

e.g. one way to do this is to only setLoading(true) if the requested settings change is not this one,

if (key !== 'toggleHideBalance') {
            setLoading(true);
        }
This revision now requires changes to proceed.Oct 31 2022, 17:01

Responding to review feedback.

This revision is now accepted and ready to land.Oct 31 2022, 21:24