Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jon/fix/balance-loading #5108

Merged
merged 5 commits into from
Jun 25, 2024
Merged

Jon/fix/balance-loading #5108

merged 5 commits into from
Jun 25, 2024

Conversation

Jon-edge
Copy link
Collaborator

@Jon-edge Jon-edge commented Jun 21, 2024

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-06-20.at.17.19.09.mp4

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

src/components/ui4/BalanceCardUi4.tsx Outdated Show resolved Hide resolved
Comment on lines 64 to 67
const progress = React.useMemo(
() => (syncableWalletIds.length === 0 ? 100 : 100 * (syncedWallets / syncableWalletIds.length)),
[syncableWalletIds.length, syncedWallets]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use a memo here. The useMemo hook needs to loop over its dependency array to see if anything has changed, which is going to cost more than just re-doing the division on each run. Math is really fast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I request that this hook return a value in the range of 0-1, and have the name useAccountSyncRatio. That makes it clearer what the units are, and mirrors the fact that the wallets themselves operate on a 0-1 scale. Visual components like the sync bar can convert the range as necessary for display purposes (like 0% - 100%).

@Jon-edge Jon-edge force-pushed the jon/fix/balance-loading branch 7 times, most recently from 18b7236 to 19224b4 Compare June 25, 2024 00:12
@paullinator
Copy link
Member

/rebase

Cause of regression might have been due to the changes made for faster login
For indicating total balance isn't fully ready yet
cacf5b1 used the container style as a text style. Height is already locked in the container style, no need to re-apply it to the text.

Design has also requested that the balance number sits centered vertically between the title and buttons.
Move the layout measurement to the parent component
@paullinator paullinator merged commit 7aff7a3 into develop Jun 25, 2024
1 check failed
@paullinator paullinator deleted the jon/fix/balance-loading branch June 25, 2024 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants