-
Notifications
You must be signed in to change notification settings - Fork 13
fix: wallet bugs #650
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
fix: wallet bugs #650
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,10 +112,10 @@ export const ConfirmClaimLinkView = ({ | |
|
||
return ( | ||
<div> | ||
<FlowHeader onPrev={onPrev} disableBackBtn={isLoading} /> | ||
<FlowHeader onPrev={onPrev} disableBackBtn={isLoading} disableWalletHeader /> | ||
<Card> | ||
<Card.Header> | ||
<Card.Title> | ||
<Card.Title className="mx-auto text-center"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: shouldn't the title be left-aligned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nah nah, its the token stuff on confirm screen, needs to be centred, and to mention we revamping it to current prod designs so confirm views gonna change |
||
<AddressLink address={claimLinkData.senderAddress} /> <br /> sent you{' '} | ||
<label className="text-start text-h2"> | ||
{claimLinkData.tokenAmount} {claimLinkData.tokenSymbol} <br /> on{' '} | ||
|
@@ -148,7 +148,7 @@ export const ConfirmClaimLinkView = ({ | |
</> | ||
)} | ||
{selectedRoute ? ( | ||
<div className="flex w-full flex-row items-start justify-center gap-1 text-h7"> | ||
<div className="flex w-full flex-row items-start justify-start gap-1 text-h7"> | ||
You are claiming{' '} | ||
{utils.formatTokenAmount( | ||
utils.formatAmountWithDecimals({ | ||
|
@@ -160,15 +160,15 @@ export const ConfirmClaimLinkView = ({ | |
{supportedSquidChainsAndTokens[selectedRoute.route.params.toChain]?.axelarChainName} | ||
</div> | ||
) : ( | ||
<div className="flex w-full flex-row items-start justify-center gap-1 text-h7"> | ||
<div className="flex w-full flex-row items-center justify-start gap-1 text-h7"> | ||
{utils.formatTokenAmount(Number(claimLinkData.tokenAmount))} {claimLinkData.tokenSymbol} on{' '} | ||
{ | ||
consts.supportedPeanutChains.find((chain) => chain.chainId === claimLinkData.chainId) | ||
?.name | ||
} | ||
</div> | ||
)} | ||
<div className="flex w-full flex-row items-center justify-center gap-1 px-2"> | ||
<div className="flex w-full flex-row items-center justify-start gap-1"> | ||
<label className="text-h7 font-normal">Claiming to:</label> | ||
<span className="flex items-center gap-1 "> | ||
<label className="text-h7"> | ||
|
@@ -180,7 +180,7 @@ export const ConfirmClaimLinkView = ({ | |
|
||
<div className="flex w-full flex-col items-center justify-center gap-2"> | ||
{selectedRoute && ( | ||
<div className="flex w-full flex-row items-center justify-between px-2 text-h8 text-grey-1"> | ||
<div className="flex w-full flex-row items-center justify-between text-h8 text-grey-1"> | ||
<div className="flex w-max flex-row items-center justify-center gap-1"> | ||
<Icon name={'forward'} className="h-4 fill-grey-1" /> | ||
<label className="font-bold">Route</label> | ||
|
@@ -215,7 +215,7 @@ export const ConfirmClaimLinkView = ({ | |
</div> | ||
)} | ||
|
||
<div className="flex w-full flex-row items-center justify-between px-2 text-h8 text-grey-1"> | ||
<div className="flex w-full flex-row items-center justify-between text-h8 text-grey-1"> | ||
<div className="flex w-max flex-row items-center justify-center gap-1"> | ||
<Icon name={'gas'} className="h-4 fill-grey-1" /> | ||
<label className="font-bold">Fees</label> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { Card } from '@/components/0_Bruddle' | |
import Icon from '@/components/Global/Icon' | ||
import { useWallet } from '@/hooks/wallet/useWallet' | ||
import { IWallet, WalletProviderType } from '@/interfaces' | ||
import { printableUsdc, shortenAddressLong } from '@/utils' | ||
import { formatExtendedNumber, printableUsdc, shortenAddressLong } from '@/utils' | ||
import { identicon } from '@dicebear/collection' | ||
import { createAvatar } from '@dicebear/core' | ||
import classNames from 'classnames' | ||
|
@@ -151,13 +151,13 @@ export function WalletCard({ type, onClick, ...props }: WalletCardProps) { | |
</div> | ||
|
||
<div className="flex items-center gap-3"> | ||
<p className="min-w-28 text-4xl font-black leading-none sm:text-5xl"> | ||
<p className="min-w-28 text-4xl font-black leading-none sm:text-[2.5rem]"> | ||
{isBalanceHidden ? ( | ||
<span className="inline-flex items-center"> | ||
<span className="relative top-1">* * * *</span> | ||
</span> | ||
) : ( | ||
`$ ${printableUsdc(wallet.balance)}` | ||
`$ ${formatExtendedNumber(printableUsdc(wallet.balance))}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought: honestly im not sure printableUsdc is a good function. Feels like we should merge it with our formatting functions. on top off that it doesnt work on bnb where usdc has 18 decimals There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. umm, alright that's a valid point, ideally my intention is to use only 1 or 2 same formatting fns moving forward and not use any other, checked the printableUsdc code, it's using viem's formatUnits method only for 6 decimals, I'll check this on my end and update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gonna look into this one, and if there's any solid action item, I'll take care of it in another pr |
||
)} | ||
</p> | ||
<button onClick={onToggleBalanceVisibility}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { formatAmount } from '../general.utils' | ||
import { formatAmount, formatExtendedNumber } from '../general.utils' | ||
|
||
describe('Amount Formatting Utilities', () => { | ||
describe('formatAmount', () => { | ||
|
@@ -70,4 +70,107 @@ describe('Amount Formatting Utilities', () => { | |
}) | ||
}) | ||
}) | ||
|
||
describe('formatExtendedNumber', () => { | ||
describe('edge cases', () => { | ||
it('should handle empty string', () => { | ||
expect(formatExtendedNumber('')).toBe('0') | ||
}) | ||
|
||
it('should handle invalid string input', () => { | ||
expect(formatExtendedNumber('invalid')).toBe('0') | ||
}) | ||
|
||
it('should handle NaN', () => { | ||
expect(formatExtendedNumber(NaN)).toBe('0') | ||
}) | ||
|
||
Comment on lines
+76
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: whats youur reasoning why it should format these and not throw instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. umm rendering NaN in ui might not be a better choice imo |
||
it('should handle undefined and null', () => { | ||
expect(formatExtendedNumber(undefined as any)).toBe('0') | ||
expect(formatExtendedNumber(null as any)).toBe('0') | ||
}) | ||
}) | ||
|
||
describe('numbers with 6 or fewer digits', () => { | ||
it('should not apply suffix for numbers with 6 or fewer digits', () => { | ||
expect(formatExtendedNumber(12345)).toBe('12345') | ||
expect(formatExtendedNumber(999)).toBe('999') | ||
expect(formatExtendedNumber(1000)).toBe('1000') | ||
expect(formatExtendedNumber(999999)).toBe('999999') | ||
}) | ||
|
||
it('should handle decimal numbers with 6 or fewer total digits', () => { | ||
expect(formatExtendedNumber(12.34)).toBe('12.34') | ||
expect(formatExtendedNumber(123.4)).toBe('123.4') | ||
expect(formatExtendedNumber(0.123)).toBe('0.12') | ||
expect(formatExtendedNumber(1234.5)).toBe('1234.5') | ||
}) | ||
|
||
it('should handle negative numbers with 6 or fewer digits', () => { | ||
expect(formatExtendedNumber(-1234)).toBe('-1234') | ||
expect(formatExtendedNumber(-12.34)).toBe('-12.34') | ||
expect(formatExtendedNumber(-99999)).toBe('-99999') | ||
}) | ||
|
||
it('should handle string inputs with 6 or fewer digits', () => { | ||
expect(formatExtendedNumber('12345')).toBe('12345') | ||
expect(formatExtendedNumber('-1234')).toBe('-1234') | ||
expect(formatExtendedNumber('999.99')).toBe('999.99') | ||
}) | ||
}) | ||
|
||
describe('numbers exceeding 6 digits', () => { | ||
it('should format whole numbers exceeding 6 digits', () => { | ||
expect(formatExtendedNumber(1234567)).toBe('1.23M') | ||
expect(formatExtendedNumber(9876543)).toBe('9.88M') | ||
}) | ||
|
||
it('should format decimal numbers exceeding 6 total digits', () => { | ||
expect(formatExtendedNumber(1234.567)).toBe('1.23K') | ||
expect(formatExtendedNumber(12345.67)).toBe('12.35K') | ||
Comment on lines
+128
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. v nice, was worried about these cases, lgtm |
||
}) | ||
|
||
it('should format negative numbers exceeding 6 digits', () => { | ||
expect(formatExtendedNumber(-1234567)).toBe('-1.23M') | ||
expect(formatExtendedNumber(-1234.567)).toBe('-1.23K') | ||
}) | ||
}) | ||
|
||
describe('suffix selection', () => { | ||
it('should apply K suffix for appropriate ranges', () => { | ||
expect(formatExtendedNumber(1234567)).toBe('1.23M') | ||
expect(formatExtendedNumber(1234.567)).toBe('1.23K') | ||
}) | ||
|
||
it('should apply M suffix for appropriate ranges', () => { | ||
expect(formatExtendedNumber(12345678)).toBe('12.35M') | ||
expect(formatExtendedNumber(123456789)).toBe('123.46M') | ||
}) | ||
|
||
it('should apply B suffix for appropriate ranges', () => { | ||
expect(formatExtendedNumber(1234567890)).toBe('1.23B') | ||
expect(formatExtendedNumber(12345678901)).toBe('12.35B') | ||
}) | ||
|
||
it('should apply T suffix for appropriate ranges', () => { | ||
expect(formatExtendedNumber(1234567890000)).toBe('1.23T') | ||
expect(formatExtendedNumber(12345678900000)).toBe('12.35T') | ||
}) | ||
}) | ||
|
||
describe('boundary cases', () => { | ||
it('should handle numbers at the 6-digit boundary', () => { | ||
expect(formatExtendedNumber(999999)).toBe('999999') | ||
expect(formatExtendedNumber(1000000)).toBe('1M') | ||
expect(formatExtendedNumber(999.9999)).toBe('1000') | ||
expect(formatExtendedNumber(999.99999)).toBe('1000') | ||
}) | ||
|
||
it('should handle numbers at suffix boundaries', () => { | ||
expect(formatExtendedNumber(999999.9)).toBe('1M') | ||
expect(formatExtendedNumber(999999999)).toBe('1B') | ||
expect(formatExtendedNumber(999999999999)).toBe('1T') | ||
}) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Shouldn't FlowHeader and WalletHeader be separate? And both be added in the confirm view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nitpicky, but id suggest:
WalletHeader
NavHeader
And then we can combine them into one FlowHeader or smth. That's how i'd think about these semantically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hugo0 FlowHeader has wallet header called inside it, along with the cta, i believe in future this will need update based on the new designs that will be made, so its fine for now