chore: add feature gate for hide balances, fix eye icon alignment #4431

finnian0826 · 2023-11-03T14:59:27Z

Description

Add feature gate for the hide balances toggle, also make naming consistent and fix alignment issue with eye icon.

Test plan

Updated unit tests

Feature gate false:

Feature gate true:

Eye icon for single token (now aligned with total):

Eye icon for multiple balances combined into total (same as before):

Related issues

N/A

Backwards compatibility

Yes

codecov · 2023-11-03T15:15:24Z

Codecov Report

Merging #4431 (d6bd7d2) into main (6c99205) will increase coverage by 0.02%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##             main    #4431      +/-   ##
==========================================
+ Coverage   85.08%   85.10%   +0.02%     
==========================================
  Files         711      711              
  Lines       26389    26400      +11     
  Branches     3577     3580       +3     
==========================================
+ Hits        22453    22468      +15     
+ Misses       3873     3869       -4     
  Partials       63       63

Files	Coverage Δ
src/app/actions.ts	`96.25% <100.00%> (ø)`
src/components/TokenBalance.tsx	`96.62% <100.00%> (+0.09%)`	⬆️
src/statsig/constants.ts	`100.00% <ø> (ø)`
src/statsig/types.ts	`100.00% <100.00%> (ø)`
src/transactions/feed/SwapFeedItem.tsx	`94.11% <100.00%> (+0.56%)`	⬆️
src/transactions/feed/TransferFeedItem.tsx	`94.73% <100.00%> (+0.45%)`	⬆️
src/app/reducers.ts	`20.27% <0.00%> (ø)`

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c99205...d6bd7d2. Read the comment docs.

cajubelt

there should be tests that verify the toggle goes away and balances are shown if the feature flag is shut off, even if the hide home balances selector gives a falsy result (so users don't get stuck with their assets hidden if we turn off the hide home balances button for some reason).

Also, right now it looks like there aren't tests for the case where the feature flag is turned off.

cajubelt · 2023-11-03T23:27:00Z

src/components/TokenBalance.tsx

@@ -285,11 +289,13 @@ export function HomeTokenBalance() {
              ? styles.totalBalance
              : styles.balance
          }
-          hideBalance={hideBalance}
+          hideBalance={showHideHomeBalancesToggle ? hideBalance : false}


ok this was clever, I'm not sure I would've thought of that. If we turn off the hide home balances toggle, we need to avoid getting some users into a state where they can't un-hide their balances. Nice.

Suggested change

hideBalance={showHideHomeBalancesToggle ? hideBalance : false}

hideBalance={showHideHomeBalancesToggle && hideBalance}

code golf

cajubelt · 2023-11-03T23:30:25Z

src/transactions/feed/SwapFeedItem.tsx

+  const hideBalanceSelectorReturn = useSelector(hideHomeBalancesSelector)
+  const hideBalance = getFeatureGate(StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE)
+    ? hideBalanceSelectorReturn
+    : false


Suggested change

const hideBalanceSelectorReturn = useSelector(hideHomeBalancesSelector)

const hideBalance = getFeatureGate(StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE)

? hideBalanceSelectorReturn

: false

const hideBalanceSelectorReturn = useSelector(hideHomeBalancesSelector)

const hideBalance = getFeatureGate(StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE) && hideBalanceSelectorReturn

code golf

cajubelt · 2023-11-03T23:31:22Z

src/transactions/feed/TransferFeedItem.tsx

+  const hideBalance = getFeatureGate(StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE)
+    ? hideBalanceSelectorReturn
+    : false


Suggested change

const hideBalance = getFeatureGate(StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE)

? hideBalanceSelectorReturn

: false

const hideBalance = getFeatureGate(StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE) && hideBalanceSelectorReturn

code golf

satish-ravi · 2023-11-06T20:31:53Z

src/components/TokenBalance.tsx

@@ -77,6 +77,9 @@ function TokenBalance({
      : undefined
  const { decimalSeparator } = getNumberFormatSettings()

+  const hideBalanceSelectorReturn = useSelector(hideHomeBalancesSelector)


Suggested change

const hideBalanceSelectorReturn = useSelector(hideHomeBalancesSelector)

const hideHomeBalanceState = useSelector(hideHomeBalancesSelector)

?
here and other places.

satish-ravi · 2023-11-06T20:59:06Z

src/components/TokenBalance.tsx

+      <View style={styles.row}>
+        <Text style={style} testID={'TotalTokenBalance'}>
+          {!hideBalance && localCurrencySymbol}
+          {balanceDisplay ?? new BigNumber(0).toFormat(2)}
+        </Text>
+        {showHideHomeBalancesToggle && <HideBalanceButton hideBalance={hideBalance} />}
+      </View>


looks like a lot of boolean expressions are duplicated. Maybe you can make this DRY by having an inner render instead of just balance display?
E.g.,

const renderTotalTokenBalance(default: string) = ( <View style={styles.row}> <Text style={style} testID={'TotalTokenBalance'}> {!hideBalance && localCurrencySymbol} {hideBalance ? `XX${decimalSeparator}XX` : totalBalanceLocal?.toFormat(2) ?? default} </Text> {showHideHomeBalancesToggle && <HideBalanceButton hideBalance={hideBalance} />} </View> )

and then do renderTotalTokenBalance('-') and renderTotalTokenBalance(new BigNumber(0).toFormat(2)) in the two places. Also not sure why one uses 0 and other - for the default.

satish-ravi · 2023-11-06T21:02:41Z

src/transactions/feed/SwapFeedItem.test.tsx

@@ -32,7 +35,13 @@ jest.mock('src/web3/networkConfig', () => {
  }
 })

+jest.mocked(getFeatureGate).mockReturnValue(true)


is this required? Looks like the one in beforeEach should suffice.

Changes since last review

satish-ravi · 2023-11-07T22:54:11Z

src/components/TokenBalance.tsx

  const balanceDisplay = hideBalance ? `XX${decimalSeparator}XX` : totalBalanceLocal?.toFormat(2)

-  if (tokenFetchError || tokenFetchLoading || tokensAreStale) {
-    // Show '-' if we haven't fetched the tokens yet or prices are stale
+  const RenderTotalTokenBalance = ({ balanceDisplay }: { balanceDisplay: string }) => {


non blocking naming nit: I think we use render prefix if we call this as a function. Since its used as a component, you can drop the Render prefix

finnian0826 added 2 commits November 3, 2023 08:57

add feature gate

fba2f80

fix hook calls

36bd0bc

finnian0826 marked this pull request as ready for review November 3, 2023 15:46

MuckT previously approved these changes Nov 3, 2023

View reviewed changes

cajubelt reviewed Nov 3, 2023

View reviewed changes

finnian0826 added 2 commits November 6, 2023 12:15

add tests, fix alignmnet

a9e27d1

Merge branch 'main' into finnian0826/hide-balances-feature-gate

0c25669

finnian0826 changed the title ~~chore: add feature gate for hide balances~~ chore: add feature gate for hide balances, fix eye icon alignment Nov 6, 2023

satish-ravi reviewed Nov 6, 2023

View reviewed changes

finnian0826 added 3 commits November 6, 2023 15:46

feedback

523980b

Merge branch 'main' into finnian0826/hide-balances-feature-gate

b6334b3

Merge branch 'main' into finnian0826/hide-balances-feature-gate

39209b7

MuckT self-requested a review November 7, 2023 16:36

Merge branch 'main' into finnian0826/hide-balances-feature-gate

f324d38

satish-ravi approved these changes Nov 7, 2023

View reviewed changes

cajubelt approved these changes Nov 7, 2023

View reviewed changes

finnian0826 added 2 commits November 7, 2023 16:06

feedback

559eadb

Merge branch 'main' into finnian0826/hide-balances-feature-gate

d6bd7d2

finnian0826 merged commit 8242a73 into main Nov 8, 2023
16 checks passed

finnian0826 deleted the finnian0826/hide-balances-feature-gate branch November 8, 2023 03:50

MuckT mentioned this pull request Jan 20, 2024

Give users the option to not display the total balance on the homescreen #2296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add feature gate for hide balances, fix eye icon alignment #4431

chore: add feature gate for hide balances, fix eye icon alignment #4431

finnian0826 commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

cajubelt left a comment •

edited

Loading

cajubelt Nov 3, 2023

cajubelt Nov 3, 2023

cajubelt Nov 3, 2023

cajubelt Nov 3, 2023

satish-ravi Nov 6, 2023

satish-ravi Nov 6, 2023 •

edited

Loading

satish-ravi Nov 6, 2023

satish-ravi Nov 7, 2023

	hideBalance={showHideHomeBalancesToggle ? hideBalance : false}
	hideBalance={showHideHomeBalancesToggle && hideBalance}

	const hideBalanceSelectorReturn = useSelector(hideHomeBalancesSelector)
	const hideHomeBalanceState = useSelector(hideHomeBalancesSelector)

chore: add feature gate for hide balances, fix eye icon alignment #4431

chore: add feature gate for hide balances, fix eye icon alignment #4431

Conversation

finnian0826 commented Nov 3, 2023 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

cajubelt left a comment • edited Loading

Choose a reason for hiding this comment

cajubelt Nov 3, 2023

Choose a reason for hiding this comment

cajubelt Nov 3, 2023

Choose a reason for hiding this comment

cajubelt Nov 3, 2023

Choose a reason for hiding this comment

cajubelt Nov 3, 2023

Choose a reason for hiding this comment

satish-ravi Nov 6, 2023

Choose a reason for hiding this comment

satish-ravi Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

satish-ravi Nov 6, 2023

Choose a reason for hiding this comment

satish-ravi Nov 7, 2023

Choose a reason for hiding this comment

finnian0826 commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

cajubelt left a comment •

edited

Loading

satish-ravi Nov 6, 2023 •

edited

Loading