Skip to content
New issue

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

#

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

Already on GitHub? # to your account

fix: number is formatted wrong when a dot is a grouping separator #6428

Merged
merged 1 commit into from
Jan 21, 2025
Merged

fix: number is formatted wrong when a dot is a grouping separator #6428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/TokenEnterAmount.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ describe('TokenEnterAmount', () => {
expect(unformatNumberForProcessing('')).toBe('')
expect(unformatNumberForProcessing('0.25')).toBe('0.25')
expect(unformatNumberForProcessing('1,234.34567')).toBe('1234.34567')
expect(unformatNumberForProcessing('1234.34567')).toBe('1234.34567')

expect(roundFiatValue(null)).toBe('')
expect(roundFiatValue(new BigNumber('0.01'))).toBe('0.01')
Expand Down Expand Up @@ -119,6 +120,7 @@ describe('TokenEnterAmount', () => {
expect(unformatNumberForProcessing('')).toBe('')
expect(unformatNumberForProcessing('0,25')).toBe('0.25')
expect(unformatNumberForProcessing('1.234,34567')).toBe('1234.34567')
expect(unformatNumberForProcessing('1234.34567')).toBe('1234.34567')

expect(roundFiatValue(null)).toBe('')
expect(roundFiatValue(new BigNumber('0.01'))).toBe('0.01')
Expand Down
13 changes: 13 additions & 0 deletions src/components/TokenEnterAmount.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,19 @@ export function formatNumber(value: string) {

export function unformatNumberForProcessing(value: string) {
const { decimalSeparator, groupingSeparator } = getNumberFormatSettings()

/**
* If the number passed is a regular number which is formatted in the standard JS number format
* (e.g. "123456.789") then just keep it as is. This will ensure this function will properly
* unformat different numbers, including those that come from external sources (e.g from API
* response)
*
* Number.isNaN considers unfinished decimal number e.g. "1." a valid number. If the number ends with grouping separator instead of decimal separator - it can be simply erased by casting it to a number.
*/
if (!Number.isNaN(+value)) {
return value.endsWith(groupingSeparator) ? `${+value}` : value
}
Comment on lines +65 to +67
Copy link
Member

@jeanregisser jeanregisser Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I'm not a big fan of the +value coercion trick as it can be confusing to read. I recommend we use BigNumber for number parsing as it's the most predictable and supports arbitrary long numbers with no precision loss.

I think it could be simplified to this:

Suggested change
if (!Number.isNaN(+value)) {
return value.endsWith(groupingSeparator) ? `${+value}` : value
}
const numericValue = new BigNumber(value);
if (!numericValue.isNaN()) {
return numericValue.toFixed()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser I'll check if that's gonna be a 1 to 1 alternative and if so - will create another PR with it!


return value.replaceAll(groupingSeparator, '').replaceAll(decimalSeparator, '.')
}

Expand Down
Loading