utils: use app locale for number separators#3927
utils: use app locale for number separators#3927cherry-1729-9090 wants to merge 1 commit intoZeusLN:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a locale-aware number formatting and parsing system, replacing legacy separatorSwap logic and manual string manipulations with centralized utilities in UnitsUtils.ts. These changes ensure consistent use of decimal and group separators across the application, including in the Amount, AmountInput, and CurrencyConverter components. The Amount component was also updated to fetch fiat rates when relevant settings change. Feedback suggests adding explanatory comments to the splitNumericValue function to improve the maintainability of its complex parsing heuristics.
| const splitNumericValue = (value: string | number, locale?: string) => { | ||
| const { decimalSeparator, groupSeparator } = | ||
| getNumberFormatSettings(locale); | ||
| const input = `${value ?? 0}`.trim().replace(/\s/g, ''); | ||
|
|
||
| if (!input) { | ||
| return { | ||
| sign: '', | ||
| integerPart: '0', | ||
| decimalPart: undefined, | ||
| hasExplicitDecimal: false | ||
| }; | ||
| } | ||
|
|
||
| const sign = input.startsWith('-') ? '-' : ''; | ||
| const unsignedInput = sign ? input.slice(1) : input; | ||
| const fallbackSeparator = decimalSeparator === '.' ? ',' : '.'; | ||
|
|
||
| const normalizeGroupedValue = (integerValue: string) => | ||
| integerValue.replace(/[.,]/g, '') || '0'; | ||
|
|
||
| const splitOnSeparator = (separator: string) => { | ||
| const parts = unsignedInput.split(separator); | ||
|
|
||
| if (parts.length === 1) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (parts.length > 2) { | ||
| const decimalPart = parts.pop() || ''; | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(parts.join(separator)), | ||
| decimalPart: decimalPart.replace(/[.,]/g, ''), | ||
| hasExplicitDecimal: true | ||
| }; | ||
| } | ||
|
|
||
| const [integerPart, decimalPart = ''] = parts; | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(integerPart), | ||
| decimalPart: decimalPart.replace(/[.,]/g, ''), | ||
| hasExplicitDecimal: true | ||
| }; | ||
| }; | ||
|
|
||
| if (/[.,]/.test(unsignedInput)) { | ||
| const lastDecimalIndex = unsignedInput.lastIndexOf(decimalSeparator); | ||
| const lastFallbackIndex = unsignedInput.lastIndexOf(fallbackSeparator); | ||
|
|
||
| if (lastDecimalIndex >= 0 && lastFallbackIndex >= 0) { | ||
| const decimalIndex = Math.max(lastDecimalIndex, lastFallbackIndex); | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue( | ||
| unsignedInput.slice(0, decimalIndex) | ||
| ), | ||
| decimalPart: unsignedInput | ||
| .slice(decimalIndex + 1) | ||
| .replace(/[.,]/g, ''), | ||
| hasExplicitDecimal: true | ||
| }; | ||
| } | ||
|
|
||
| if (unsignedInput.endsWith(decimalSeparator)) { | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(unsignedInput.slice(0, -1)), | ||
| decimalPart: '', | ||
| hasExplicitDecimal: true | ||
| }; | ||
| } | ||
|
|
||
| if (unsignedInput.includes(decimalSeparator)) { | ||
| return splitOnSeparator(decimalSeparator)!; | ||
| } | ||
|
|
||
| if (unsignedInput.includes(groupSeparator)) { | ||
| const parts = unsignedInput.split(groupSeparator); | ||
| const [integerPart, decimalPart = ''] = parts; | ||
| const isDecimalFallback = | ||
| parts.length === 2 && | ||
| (integerPart === '0' || decimalPart.length !== 3); | ||
|
|
||
| if (isDecimalFallback) { | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(integerPart), | ||
| decimalPart: decimalPart.replace(/[.,]/g, ''), | ||
| hasExplicitDecimal: true | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(parts.join(groupSeparator)), | ||
| decimalPart: undefined, | ||
| hasExplicitDecimal: false | ||
| }; | ||
| } | ||
|
|
||
| if (unsignedInput.endsWith(fallbackSeparator)) { | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(unsignedInput.slice(0, -1)), | ||
| decimalPart: '', | ||
| hasExplicitDecimal: true | ||
| }; | ||
| } | ||
|
|
||
| if (unsignedInput.includes(fallbackSeparator)) { | ||
| const parts = unsignedInput.split(fallbackSeparator); | ||
| const [integerPart, decimalPart = ''] = parts; | ||
| const isGroupedFallback = | ||
| parts.length > 2 || | ||
| (parts.length === 2 && | ||
| decimalPart.length === 3 && | ||
| integerPart.length > 0 && | ||
| integerPart !== '0'); | ||
|
|
||
| if (isGroupedFallback) { | ||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue( | ||
| parts.join(fallbackSeparator) | ||
| ), | ||
| decimalPart: undefined, | ||
| hasExplicitDecimal: false | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| sign, | ||
| integerPart: normalizeGroupedValue(integerPart), | ||
| decimalPart: decimalPart.replace(/[.,]/g, ''), | ||
| hasExplicitDecimal: true | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| sign, | ||
| integerPart: unsignedInput.replace(/[.,]/g, '') || '0', | ||
| decimalPart: undefined, | ||
| hasExplicitDecimal: false | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This function splitNumericValue is quite complex due to the various heuristics for parsing locale-specific number formats. While it appears to handle many cases correctly, its complexity could make it difficult to maintain or debug in the future.
To improve maintainability, please consider adding comments to explain the logic, especially for the fallback heuristics like isDecimalFallback and isGroupedFallback. Explaining why these checks are structured the way they are (e.g., why decimalPart.length !== 3 is a deciding factor) would be very helpful for future developers.
| componentDidMount() { | ||
| this.ensureFiatRatesAvailable(); | ||
| } | ||
|
|
||
| componentDidUpdate(prevProps: Readonly<AmountProps>) { | ||
| if ( | ||
| prevProps.fixedUnits !== this.props.fixedUnits || | ||
| prevProps.sats !== this.props.sats || | ||
| prevProps.FiatStore?.fiatRates !== | ||
| this.props.FiatStore?.fiatRates || | ||
| prevProps.SettingsStore?.settings.fiat !== | ||
| this.props.SettingsStore?.settings.fiat | ||
| ) { | ||
| this.ensureFiatRatesAvailable(); | ||
| } | ||
| } | ||
|
|
||
| ensureFiatRatesAvailable = () => { | ||
| const FiatStore = this.props.FiatStore!; | ||
| const UnitsStore = this.props.UnitsStore!; | ||
| const SettingsStore = this.props.SettingsStore!; | ||
| const units = this.props.fixedUnits || UnitsStore.units; | ||
| const selectedFiat = SettingsStore.settings?.fiat; | ||
|
|
||
| if ( | ||
| units !== 'fiat' || | ||
| !SettingsStore.settings?.fiatEnabled || | ||
| !selectedFiat || | ||
| FiatStore.loading | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const hasSelectedFiatRate = FiatStore.fiatRates?.some( | ||
| (entry: any) => entry.code === selectedFiat | ||
| ); | ||
|
|
||
| if (!hasSelectedFiatRate) { | ||
| FiatStore.getFiatRates(); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
why are these changes needed?
There was a problem hiding this comment.
These changes were added because, while testing this PR, I found that some places render before the selected fiat rate is available in FiatStore, which causes the component to show N/A even though the formatting logic is correct. The new guard only runs when the component is actually rendering fiat, fiat is enabled, a fiat currency is selected, the selected rate is missing, and a fetch is not already in progress, so it is just a defensive way to avoid that missing-rate state in amount/conversion flows that do not always go through the main wallet refresh path.
|
I don't think this logic should apply to fiat currencies, which have their own standards for comma vs decimal formatting, that we already define very clearly in the app. I think we should only apply this for Bitcoin and satoshi amounts. What do you think @myxmaster? |
|
First of all thanks a lot @cherry-1729-9090 for taking the time and working on this, it is a pretty important issue imo. I did not test this yet or looked into the code. But concept-wise, I strongly suggest keeping sats/₿ and fiat in line, meaning: when the app language is English, use English thousands and decimal separators for everything. The formatting should follow the reader's locale, not the currency. Mixing conventions within the same screen is cognitively jarring and can lead to misreads. (Example: When you're on vacation in Europe and see € 1.021,34, would that feel intuitive to you, or would you prefer € 1,021.34 right next to your 1,755,264 sats?) |
|
Thanks a lot for the feedback, really appreciate it. That’s the approach implemented in this PR to avoid mixed formatting on the same screen and reduce misreads. |
cherry-1729-9090
force-pushed
the
feature/app-language-number-separators
branch
from
f4b259c to
c00b779
Compare
cherry-1729-9090
force-pushed
the
feature/app-language-number-separators
branch
from
c00b779 to
2ad4c34
Compare
3 participants
Description
Relates to issue: #3833
This PR makes number separators follow the app language across the app.
Before this change, Bitcoin and sats were always shown in English-style formatting, while fiat formatting depended on the selected currency. That could show mixed separator styles on the same screen.
With this change, the selected app language is used as the single source of truth for number separators for:
This keeps formatting consistent when switching between languages like English and German.
This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: