Fix format amount review modal #1265
Conversation
Deploying with
|
| Latest commit: |
81b6f02
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3c72d04e.web-core.pages.dev |
| Branch Preview URL: | https://fix-format-amount-review-mod.web-core.pages.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Branch preview |
| @@ -268,4 +268,24 @@ describe('formatNumber', () => { | |||
| expect(formatCurrency(amount4, 'BHD')).toBe('< -0.001 BHD') | |||
| }) | |||
| }) | |||
|
|
|||
| describe('localeNumberFormatter', () => { | |||
There was a problem hiding this comment.
We should add international tests.
src/utils/formatNumber.ts
Outdated
|
|
||
| export const localeNumberFormatter = (amount: number | string): string => { | ||
| // get locale decimal separator | ||
| const number = 123456.789 |
There was a problem hiding this comment.
Using the arbitrary value confused me at first. I would suggest using Intl.NumberFormat.prototype.formatToParts instead as you can directly get the separators from amount directly.
src/utils/formatNumber.ts
Outdated
| // trailing zeros | ||
| const decimalFinalWithoutTrailingZeros = decimalString ? decimalFinal.replace(/0+$/, '') : '' | ||
|
|
||
| const integerFinal = integerString.replace(/\B(?=(\d{3})+(?!\d))/g, thousandSeparator) |
There was a problem hiding this comment.
Are thousand separators universal/set every three digits in all number formats?
There was a problem hiding this comment.
Definitely not: https://learn.microsoft.com/en-us/globalization/locale/number-formatting
Hindi – two digits, Japanese four.
There was a problem hiding this comment.
Good point here, the regex was very opinionated. I'm using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString now to format amount's integer part.
DiogoSoaress
force-pushed
the
fix-format-amount-review-modal
branch
from
81b6f02 to
3cfcd5b
Compare
Branch preview✅ Deploy successful! https://fix-format-amount-review-modal--webcore.review-web-core.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Looks good! Noticed a different bug when testing this. The same amount in the select and input below is formatted differently:
|
|
Using our utils formatter does not give a very precise view to the amount of tokens to be transferred so I've created a new ticket for this enhancement #1321 |
I'm going to create a new ticket to address that bug Edit: |
| @@ -26,7 +27,7 @@ export const TokenTransferReview = ({ | |||
|
|
|||
| <Box mt={1} fontSize={20}> | |||
| {children} | |||
| {amount} {tokenInfo.symbol} | |||
| {formatAmount(amount)} {tokenInfo.symbol} | |||
There was a problem hiding this comment.
I wouldn't use the current formatter for the reasons you stated here. This is arguably less clear than non-formatted numbers.
There was a problem hiding this comment.
I won't merge this PR until we discuss it tomorrow
There was a problem hiding this comment.
I tend to agree it might be worse than no formatting in some (rather extreme?) cases.
There was a problem hiding this comment.
On the other hand, this is how we format numbers in other places in the app, like it or not. I don’t see why this modal has to be different than tx list, balances etc.
There was a problem hiding this comment.
This is the one place that I would argue we always display the exact precision though as assets have aribtrary values. If I were sending 100,400, I would much sooner "review" that than 100K. The same with decimal truncation.
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2022 Safe Web Core Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
What it solves
Resolves #705
How this PR fixes it
Implements a new formatter
formatAmountPreciseto format a number with a passed precision without adhering to our style guide notation.Uses the app
formatAmountPreciseformatter for review modal header's amounts.How to test it
Analytics changes
N/A
Screenshots