feat: hide balance #5830

adrianocola · 2024-08-30T15:07:57Z

This PR:

Fixes Provide option to hide values #5096 .
Instead of hiding balance when clicking the balance text, an eye icon was added to hide/show the balance.
The total USD balance, assets balances, and the values of activities can be hidden.
Created integration test.

Implementation details:

Created HideableBalance component that shows/hides its child based on the HideBalanceContext value.
Some components that show balance are used everywhere in the code, but only the balances on the home screen should be hidden, so the visibility of balances was conditioned to a React Context.
Created HideBalanceProvider and added it to the home page.
The hide/show status is stored in Redux and thus persists between restarts.

Please let me know if there’s anything I need to change!

Wallet:

Screen.Recording.2024-08-27.at.14.32.13.mov

Large Screen:

Screen.Recording.2024-08-27.at.14.33.45.mov

adrianocola · 2024-08-30T15:09:41Z

src/app/ui/components/account/account.card.tsx

+            </Box>
+          </Flex>
+        </Link>
+        <Box mr={{ md: 'space.05' }}>


Added this margin to align with the other buttons ("large screen" only)

adrianocola · 2024-08-30T15:11:04Z

src/app/common/hide-balance-provider.tsx

Not sure if this file should be in the "common" directory. Is there a better place?

pete-watters · 2024-09-02T05:12:31Z

Thanks for the contribution @adrianocola , its much appreciated.

I'll check it out now. In the meantime can you please:

rebase your work with the latest from dev (rebase instead of merge)
squash your commits to a single commit
run prettier to fix lint issues failing in lint-prettier

Thanks!

adrianocola · 2024-09-02T12:11:03Z

Hey @pete-watters, thank you for the feedback. I just made the changes you asked for. Let me know if there is anything else I need to change!

src/app/common/hide-balance-provider.tsx

pete-watters · 2024-09-02T15:14:13Z

Hey @pete-watters, thank you for the feedback. I just made the changes you asked for. Let me know if there is anything else I need to change!

Thanks @adrianocola 👍 I checked with the team and their feedback was that it would be better to store this setting in redux only and avoid using an extra context. Could you refactor it a bit to avoid the extra context?

Otherwise the code looks pretty good to me. Thanks again!

adrianocola · 2024-09-02T17:16:16Z

Thanks for the team input, @pete-watters!

I've added the Context to hide the balances that are only visible on the Home screen. The components that show the balance on the Home screen (like CryptoAssetItemLayout) are used on many other screens. Without the context, those balances would be hidden everywhere.

I can hide the balance everywhere in the extension. Some approaches:

Hide/Show Balance Only from the Home Screen
While this approach ensures that the visibility control is limited to the Home screen, it might frustrate users. They would need to navigate back to the Home screen every time they wish to toggle the visibility.
Hide/Show Balance Everywhere
If we extend the balance visibility toggle across all screens, the “eye” icon should be accessible on every screen. The obvious place would be the header, but it doesn't feel right. The header is also not always visible (full-screen popups hide the header). An alternative could be to display the “eye” icon in the header only when the balances are hidden, allowing users to reveal them from almost any screen.
Toggle Balance Visibility by Clicking the Hidden Balance Dots
Another option is to allow users to toggle the visibility by clicking on the hidden balance dots (•••) wherever they appear. However, this introduces complexity, especially on components with interactive/clickable elements like activity items. Making the dots clickable (make them stand out on hover) or adding a small “eye” icon next to them (on hover) could be solutions, but this might lead to usability or accessibility issues.

To maintain the current solution where balances are hidden only on the Home screen, I can modify some components, such as AssetList and ActivityList, to accept an optional hideBalance prop. This prop can then be passed down the component tree until it reaches the actual balance component.

Please let me know what you prefer or if you have any other suggestions!

pete-watters · 2024-09-03T09:46:41Z

Thanks for the team input, @pete-watters!

I've added the Context to hide the balances that are only visible on the Home screen. The components that show the balance on the Home screen (like CryptoAssetItemLayout) are used on many other screens. Without the context, those balances would be hidden everywhere.

I can hide the balance everywhere in the extension. Some approaches:

Hide/Show Balance Only from the Home Screen
While this approach ensures that the visibility control is limited to the Home screen, it might frustrate users. They would need to navigate back to the Home screen every time they wish to toggle the visibility.

Hide/Show Balance Everywhere
If we extend the balance visibility toggle across all screens, the “eye” icon should be accessible on every screen. The obvious place would be the header, but it doesn't feel right. The header is also not always visible (full-screen popups hide the header). An alternative could be to display the “eye” icon in the header only when the balances are hidden, allowing users to reveal them from almost any screen.

Toggle Balance Visibility by Clicking the Hidden Balance Dots
Another option is to allow users to toggle the visibility by clicking on the hidden balance dots (•••) wherever they appear. However, this introduces complexity, especially on components with interactive/clickable elements like activity items. Making the dots clickable (make them stand out on hover) or adding a small “eye” icon next to them (on hover) could be solutions, but this might lead to usability or accessibility issues.

To maintain the current solution where balances are hidden only on the Home screen, I can modify some components, such as AssetList and ActivityList, to accept an optional hideBalance prop. This prop can then be passed down the component tree until it reaches the actual balance component.

Please let me know what you prefer or if you have any other suggestions!

Thank you for your hard work @adrianocola 💪

I spoke to the dev team and the design team and we think it's best to keep this simple and manage the state globally with redux, your option 2 above. Our suggestion is to:

remove the use of a specific context wrapper
store the privacy setting globally in redux
toggle privacy mode via the settings menu (get the eye to open / close based on status)

adrianocola · 2024-09-03T11:14:43Z

@pete-watters, I forgot about the menu! This is definitively the best solution!

Should I create a new commit for this, or would you prefer I continue squashing them?

Edit: I made the change and kept everything in a single commit!

pete-watters · 2024-09-03T15:47:31Z

@pete-watters, I forgot about the menu! This is definitively the best solution!

Should I create a new commit for this, or would you prefer I continue squashing them?

Edit: I made the change and kept everything in a single commit!

That's great, thank you. One commit is best 👍

I'll test it soon and hopefully get it merged. Thanks!

alter-eggo · 2024-09-04T09:18:38Z

@markmhendrickson @pete-watters we prob need to hide balances here as well? wdyt

pete-watters · 2024-09-04T10:34:42Z

@markmhendrickson @pete-watters we prob need to hide balances here as well? wdyt

I think we could for stacked balances but not in the popup header. In the popup you don't always have access to the settings menu so you won't be able to easily show your balance again.

It could be nice to hide it everywhere though

pete-watters · 2024-09-04T10:37:15Z

Thanks @adrianocola 👍 I tested this and it LGTM.

I checked with the design team and they asked if you can get rid of the toggle 👁️ on the account card and just have it in the Settings menu.

We also have an open Q here about hiding it in more places #5830 (comment).

It should be good to merge if you could please:

remove the eye from the Account card
update based on the decision on hiding more balances

Thanks for your patience here and for your contributions

markmhendrickson · 2024-09-04T11:02:05Z

I think we could for stacked balances but not in the popup header. In the popup you don't always have access to the settings menu so you won't be able to easily show your balance again.

It does seem better to hide consistently, even if some places (like the popup header) don't allow toggling per se. The user can open up the extension home and toggle from there as desired, even if not directly in the popup.

adrianocola · 2024-09-04T11:31:07Z

Hey, team! Thank you for the input; I'll do it, @pete-watters! 💪

I also like the idea of consistently hiding everywhere. How about allowing the user to click the hidden balance in the header to show the balance? I can even extend this behavior to other places (where the balance doesn't show over a clickable button) to provide an easy way to show balances.

Screen.Recording.2024-09-04.at.08.27.32.mov

pete-watters · 2024-09-05T07:37:11Z

Hey, team! Thank you for the input; I'll do it, @pete-watters! 💪

I also like the idea of consistently hiding everywhere. How about allowing the user to click the hidden balance in the header to show the balance? I can even extend this behavior to other places (where the balance doesn't show over a clickable button) to provide an easy way to show balances.

Screen.Recording.2024-09-04.at.08.27.32.mov

Good idea @adrianocola 👍 I checked with the design team and they would be happy if you can go ahead with this.

Thanks again for your hard work

mica000 · 2024-09-05T14:24:05Z

Shall we use *** instead of ••• as the dot seems to get confused with our ellipses?

adrianocola · 2024-09-05T14:48:59Z

Shall we use *** instead of ••• as the dot seems to get confused with our ellipses?

I used dots because asterisks can change drastically depending on the font. An example:

But I can override and enforce another font, like this example using Diatype for the ***:

Should I keep the *** using Diatype font (or any other font)?

mica000 · 2024-09-06T14:09:15Z

Shall we use *** instead of ••• as the dot seems to get confused with our ellipses?

I used dots because asterisks can change drastically depending on the font. An example:

But I can override and enforce another font, like this example using Diatype for the ***:

Should I keep the *** using Diatype font (or any other font)?

Good idea! Lets keep Diatype. Thanks for the suggestion!

adrianocola · 2024-09-09T19:54:35Z

Hey @pete-watters, I've made the changes proposed by the team! This is a summary with the changes:

Renaming for Consistency:
- Refactored terminology from “hide balance” to “privacy mode” for consistent alignment with the interface wording.
- Renamed component HideableBalance to PrivateText.
- Updated Redux store property from hideBalance to isPrivacyMode.
Component Refactoring:
- Split the PrivateText component into two parts:
  - private-text.layout.tsx (app/ui/components/privacy): Stateless presentation component.
  - private-text.tsx (app/components/privacy): Handles retrieving privacy mode status from the Redux store and rendering the presentation component.
UI Adjustments:
- Removed the privacy toggle icon from the account card UI.
- Added the PrivateText component to balances that previously missed this feature.
- Changed the privacy mask from ••• to *** and forced the Diatype font to maintain consistency across different parts of the extension.
New Interaction:
- Introduced a feature allowing users to reveal hidden text by clicking the ***. This functionality is enabled for balances in the following sections:
  - Account card on the homepage.
  - Account balance at the bottom of the send page.
  - Account balance at the top of the sign modal.

Let me know if there is anything else I need to change!

src/app/ui/components/privacy/private-text.layout.tsx

pete-watters · 2024-09-10T05:28:13Z

Hey @pete-watters, I've made the changes proposed by the team! This is a summary with the changes:

Renaming for Consistency:

Refactored terminology from “hide balance” to “privacy mode” for consistent alignment with the interface wording.

Renamed component HideableBalance to PrivateText.

Updated Redux store property from hideBalance to isPrivacyMode.

Component Refactoring:

Split the PrivateText component into two parts:

private-text.layout.tsx (app/ui/components/privacy): Stateless presentation component.

private-text.tsx (app/components/privacy): Handles retrieving privacy mode status from the Redux store and rendering the presentation component.

UI Adjustments:

Removed the privacy toggle icon from the account card UI.

Added the PrivateText component to balances that previously missed this feature.

Changed the privacy mask from ••• to *** and forced the Diatype font to maintain consistency across different parts of the extension.

New Interaction:

Introduced a feature allowing users to reveal hidden text by clicking the ***. This functionality is enabled for balances in the following sections:

Account card on the homepage.

Account balance at the bottom of the send page.

Account balance at the top of the sign modal.

Let me know if there is anything else I need to change!

Thanks @adrianocola 👏 It's generally looking really great, thanks for all of the hard work, it's almost there.

I mentioned one possible coding style improvement here

I noticed that for locked balances the UI is not aligned correctly, so that needs to be fixed:

adrianocola · 2024-09-10T13:35:13Z

Hey @pete-watters, thanks for the support and patience!

Fixed the "locked" alignment issue. Actually, I removed it. It doesn't make much sense to show them anyway while hiding the balance. So, whether there are locked values or not, it will always show only ***.
Using a monospaced font for the asterisks (fontFamily: 'Fira Code, Consolata, monospace'). Monospaced fonts center them vertically, making the transition between the balance text and the asterisks more natural.
Refactored the code to remove the PrivateText stateful component.
- This component was being used all over the place to hide balances easily;
- But it was also being used inside *.layout.tsx components, which broke the project Storybook;
- So I'm using the stateless PrivateTextLayout component in its place;
- I had to get the isPrivateMode state from redux everywhere a balance is displayed and pass it to the PrivateTextLayout (much like the isLoading prop is passed around);

pete-watters · 2024-09-11T05:00:42Z

src/app/components/balance/balance-private.tsx

+  canClickToShow?: boolean;
+}
+
+export function BalancePrivate({ children, canClickToShow, ...rest }: BalancePrivateProps) {


Maybe this can be PrivateBalance to match the convention of PrivateText ?

Do you need the prop canClickToShow? It seems to be always passed in as true?

I renamed the component and am now using it in a place that needs to set the prop canClickToShow=false (I actually don't pass it at all).

pete-watters · 2024-09-11T05:07:20Z

src/app/ui/components/privacy/private-text.layout.tsx

@@ -0,0 +1,38 @@
+import React from 'react';


Maybe we can introduce a container component here - PrivateText, so that it can fetch the setting from the state and avoid drilling it down?

So it would be something like private-text.tsx :

import { PrivateTextLayout } from '@app/ui/components/privacy/private-text.layout'; export function PrivateText() { const isPrivate = useIsPrivateMode(); return ( <PrivateTextLayout isPrivate={isBalancePrivate} onShowValue={onShowBalance} display="inline-block" overflow="hidden" > {balance} </PrivateTextLayout> ) }

That could help avoid having to drill down isPrivate={isPrivate} so often and we could isolate it into it's own feature: features/privacy/. We could also add onShowValue={onShowBalance} in there

My previous version had this container component, and I used it everywhere. Unfortunately, this broke the Storybook and the project structure because it was used in other layout components. The biggest ones were the CryptoAssetItemLayout and TransactionItemLayout components.

I've removed the drilling down of the isPrivate prop in the AssetList component. I've moved the state subscription closer to the CryptoAssetItemLayout or TransactionItemLayout component.

I moved the PrivateText and PrivateTextLayout components to the app/components/privacy/ folder. These components are used by other components in the components and features folders, so the dependency-cruiser did not allow me to place them in the features folder.

OK great, thanks for the explanation 👍

pete-watters · 2024-09-11T05:09:37Z

Hey @pete-watters, thanks for the support and patience!

Fixed the "locked" alignment issue. Actually, I removed it. It doesn't make much sense to show them anyway while hiding the balance. So, whether there are locked values or not, it will always show only ***.

Using a monospaced font for the asterisks (fontFamily: 'Fira Code, Consolata, monospace'). Monospaced fonts center them vertically, making the transition between the balance text and the asterisks more natural.

Refactored the code to remove the PrivateText stateful component.

This component was being used all over the place to hide balances easily;

But it was also being used inside *.layout.tsx components, which broke the project Storybook;

So I'm using the stateless PrivateTextLayout component in its place;

I had to get the isPrivateMode state from redux everywhere a balance is displayed and pass it to the PrivateTextLayout (much like the isLoading prop is passed around);

This looks great @adrianocola and it is working perfectly, nice job!

I added two more suggestions about how we could improve the design of the code but I think it's pretty solid overall and close to merge. Let me know what you think

adrianocola · 2024-09-11T13:06:59Z

@pete-watters, thanks for the suggestions. Almost there!

Moved the two main components to the folder app/components/privacy:
- private-text.layout.tsx
- private-text.tsx
Reduced the drill down of the isPrivate prop in the AssetsList tree.

pete-watters

Great work @adrianocola , thanks for your persistence here. The code looks good to me and I tested it and it's working well.

I will try to get it merged soon but we are having a CI issue now I need to fix first.

@mica000 can you check this build and do a Design QA when you have a chance?

Here's a demo in case it helps
https://github.com/user-attachments/assets/7d545fd9-b411-4b83-8cde-9f3cad3d02c8

pete-watters · 2024-09-13T08:06:29Z

@adrianocola : Great work on this feature 👏 I will get it merged and into the next release once we have resolved the CI issue

mica000 · 2024-09-13T14:06:59Z

Indeed! Thanks @adrianocola!! ✨

pete-watters · 2024-09-24T08:37:12Z

@adrianocola - I got your work merged in and we will include it in the next release. Thanks again for your efforts here 👏

adrianocola · 2024-09-24T11:44:03Z

Thank you so much, everyone! I’m really glad I could contribute and work with all of you! 🚀

adrianocola commented Aug 30, 2024

View reviewed changes

adrianocola marked this pull request as ready for review August 30, 2024 16:56

adrianocola changed the title ~~Feat/hide balance~~ feat: hide balance Aug 30, 2024

pete-watters mentioned this pull request Sep 2, 2024

feat: hide-balance #5831

Closed

pete-watters requested review from alter-eggo and pete-watters and removed request for alter-eggo September 2, 2024 05:13

pete-watters self-assigned this Sep 2, 2024

adrianocola force-pushed the feat/hide-balance branch 3 times, most recently from 7713135 to 3cc02b5 Compare September 2, 2024 12:08

pete-watters reviewed Sep 2, 2024

View reviewed changes

src/app/common/hide-balance-provider.tsx Outdated Show resolved Hide resolved

markmhendrickson requested a review from mica000 September 3, 2024 10:58

adrianocola force-pushed the feat/hide-balance branch from 3cc02b5 to 57ee6b0 Compare September 3, 2024 13:07

pete-watters mentioned this pull request Sep 4, 2024

feat: add option to hide balance, closes leather-io#5096 #5843

Closed

pete-watters force-pushed the feat/hide-balance branch 2 times, most recently from dad3fda to 3078f8f Compare September 4, 2024 09:31

adrianocola force-pushed the feat/hide-balance branch from 3078f8f to 78a7805 Compare September 4, 2024 10:31

adrianocola force-pushed the feat/hide-balance branch from 78a7805 to 604cc24 Compare September 5, 2024 16:34

adrianocola force-pushed the feat/hide-balance branch 2 times, most recently from 8bc0558 to 8d4b1cd Compare September 9, 2024 19:46

pete-watters reviewed Sep 10, 2024

View reviewed changes

src/app/ui/components/privacy/private-text.layout.tsx Outdated Show resolved Hide resolved

adrianocola force-pushed the feat/hide-balance branch 3 times, most recently from c770a25 to 49e7d00 Compare September 10, 2024 13:33

adrianocola force-pushed the feat/hide-balance branch from 49e7d00 to 459f6e5 Compare September 10, 2024 13:35

pete-watters reviewed Sep 11, 2024

View reviewed changes

feat: add option to hide balance, closes leather-io#5096

d2599ab

adrianocola force-pushed the feat/hide-balance branch from 459f6e5 to d2599ab Compare September 11, 2024 13:04

pete-watters approved these changes Sep 12, 2024

View reviewed changes

mica000 approved these changes Sep 13, 2024

View reviewed changes

pete-watters mentioned this pull request Sep 20, 2024

feat: add option to hide balance, closes leather-io#5096 #5865

Merged

pete-watters closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hide balance #5830

feat: hide balance #5830

adrianocola commented Aug 30, 2024

adrianocola Aug 30, 2024 •

edited

Loading

adrianocola Aug 30, 2024

pete-watters commented Sep 2, 2024

adrianocola commented Sep 2, 2024

pete-watters commented Sep 2, 2024

adrianocola commented Sep 2, 2024

pete-watters commented Sep 3, 2024

adrianocola commented Sep 3, 2024 •

edited

Loading

pete-watters commented Sep 3, 2024

alter-eggo commented Sep 4, 2024

pete-watters commented Sep 4, 2024

pete-watters commented Sep 4, 2024

markmhendrickson commented Sep 4, 2024

adrianocola commented Sep 4, 2024

pete-watters commented Sep 5, 2024

mica000 commented Sep 5, 2024

adrianocola commented Sep 5, 2024

mica000 commented Sep 6, 2024

adrianocola commented Sep 9, 2024

pete-watters commented Sep 10, 2024

adrianocola commented Sep 10, 2024

pete-watters Sep 11, 2024

adrianocola Sep 11, 2024

pete-watters Sep 11, 2024 •

edited

Loading

adrianocola Sep 11, 2024

pete-watters Sep 12, 2024

pete-watters commented Sep 11, 2024

adrianocola commented Sep 11, 2024

pete-watters left a comment

pete-watters commented Sep 13, 2024

mica000 commented Sep 13, 2024

pete-watters commented Sep 24, 2024

adrianocola commented Sep 24, 2024

feat: hide balance #5830

feat: hide balance #5830

Conversation

adrianocola commented Aug 30, 2024

This PR:

adrianocola Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

adrianocola Aug 30, 2024

Choose a reason for hiding this comment

pete-watters commented Sep 2, 2024

adrianocola commented Sep 2, 2024

pete-watters commented Sep 2, 2024

adrianocola commented Sep 2, 2024

pete-watters commented Sep 3, 2024

adrianocola commented Sep 3, 2024 • edited Loading

pete-watters commented Sep 3, 2024

alter-eggo commented Sep 4, 2024

pete-watters commented Sep 4, 2024

pete-watters commented Sep 4, 2024

markmhendrickson commented Sep 4, 2024

adrianocola commented Sep 4, 2024

pete-watters commented Sep 5, 2024

mica000 commented Sep 5, 2024

adrianocola commented Sep 5, 2024

mica000 commented Sep 6, 2024

adrianocola commented Sep 9, 2024

pete-watters commented Sep 10, 2024

adrianocola commented Sep 10, 2024

pete-watters Sep 11, 2024

Choose a reason for hiding this comment

adrianocola Sep 11, 2024

Choose a reason for hiding this comment

pete-watters Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

adrianocola Sep 11, 2024

Choose a reason for hiding this comment

pete-watters Sep 12, 2024

Choose a reason for hiding this comment

pete-watters commented Sep 11, 2024

adrianocola commented Sep 11, 2024

pete-watters left a comment

Choose a reason for hiding this comment

pete-watters commented Sep 13, 2024

mica000 commented Sep 13, 2024

pete-watters commented Sep 24, 2024

adrianocola commented Sep 24, 2024

adrianocola Aug 30, 2024 •

edited

Loading

adrianocola commented Sep 3, 2024 •

edited

Loading

pete-watters Sep 11, 2024 •

edited

Loading