fix/78095: Comma split on locale languages#78246
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppCleanShot.2025-12-25.at.00.57.31__2.mp4Android: mWeb ChromeCleanShot.2025-12-25.at.01.00.05__2.mp4iOS: HybridAppCleanShot.2025-12-25.at.00.53.23__2.mp4iOS: mWeb SafariCleanShot.2025-12-25.at.00.51.07__2.mp4MacOS: Chrome / SafariCleanShot.2025-12-25.at.00.55.18__2.mp4 |
src/components/PercentageForm.tsx
Outdated
| const strippedAmount = stripCommaFromAmount(newAmountWithoutSpaces); | ||
| onInputChange?.(strippedAmount); | ||
| // Convert comma to period for internal representation (commas are used as decimal separators in some locales like Spanish) | ||
| const normalizedAmount = newAmountWithoutSpaces.replace(',', '.'); |
There was a problem hiding this comment.
We can using existing func
| const normalizedAmount = newAmountWithoutSpaces.replace(',', '.'); | |
| const normalizedAmount = replaceCommasWithPeriod(newAmountWithoutSpaces); |
dangrous
left a comment
There was a problem hiding this comment.
Thank you for putting this through! Test cases brought up one question that we might need to adjust for.
| expect(validatePercentage('1,234.56', true, true)).toBe(false); | ||
| expect(validatePercentage('1.234,56', true, true)).toBe(false); |
There was a problem hiding this comment.
These are the only test cases I'm unsure about - should these be accepted (assuming allowExceedingHundred)? Both would be valid percentages, since the first punctuation is being used as a hundreds separator instead of a decimal. What would the first one 1,234.56 return BEFORE we made the changes in this branch?
I realize we may need to do some combo of replaceCommasWithPeriod and stripCommasFromAmount - maybe we swap all periods with commas or something?
There was a problem hiding this comment.
Clarifications:
-
Before this branch, the regex only allowed one period not comma(which is the whole issue), so 1,234.56 would have been obviously rejected.
Currently new regex - [.,]? optionally ONE comma or period followed by \d? optionally ONE more digit -
Regarding the concern about users entering commas as thousands separators: the input allows only a single decimal digit. This greatly limits the possibility of mixed notation or user confusion.
-
a) Percentages 0-100 should never need thousands separators as in default
b) IF allowExceedingHundred is passed, this is regex:/^\d*[.,]?\d?$/u- very permissive, allows any number with optional one decimal
The validation correctly supports either a comma or a period as the decimal separator, but not thousands separators or mixed notation, which is appropriate for percentages.
There was a problem hiding this comment.
hrm okay - @heyjennahay from a product perspective, what do you think here? Is it okay to disallow someone putting 1,234.56 into the percentage (while allowing 1234.56)? I think it's pretty much an edge case, but since we're in there changing the logic, figured I'd check. Seems like it would make it a little more complicated
There was a problem hiding this comment.
Sorry for the slow reply. I think this is fine. As you say, it is an edge case and not really worth putting a "complete" solution which handles all cases.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@btkcodedev Oops — the pipeline just finished running after the PR was approved, and we still have issues with commit signing and the prettier check that need to be resolved.
|
f5fffd6 to
4ee02d3
Compare
|
Thanks @suneox |
CC: @heyjennahay |
|
@heyjennahay Bump on previous comment |
|
This is good to go but can you merge main one more time just to make sure there isn't anything unexpected @btkcodedev ? Thanks! |
|
Done @dangrous ✅ |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.2.97-0 🚀
|
|
🚀 Deployed to staging by https://github.com/dangrous in version: 9.2.99-0 🚀
|
I just verified this on the latest staging CleanShot.2026-01-12.at.19.50.36.mp4@btkcodedev Could you please take a look this one with latest main? |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.2.99-8 🚀
|


Explanation of Change
Updating the input validation regex fixes the Split Expense issue in Spanish, allowing users to enter commas, add digits after a comma, and use the Delete key in the Percentage tab.
Fixed Issues
$ #78095
PROPOSAL: #78095 (comment)
Tests
Precondition:
App language is Spanish.
Bug 1: A comma cannot be entered in the percentage input field
Go to staging.new.expensify.com
Go to workspace chat.
Create an expense of the amount 30.00
Open expense report.
Click More > Split > Percent.
Click on any input field.
Enter a comma.
Verify that a comma can be entered now in the percentage input field
Bug 2: Numbers cannot be added after entering a comma
Continue from Bug 1.
Enter a number after the comma.
Verify that numbers can be entered after a comma.
Bug 3: The delete key does nothing when pressed
Continue from Bug 1.
In the Percent tab, press the Delete key.
Verify that the Delete key removes numbers from the field.
Offline tests
Same as above
QA Steps
Bug 1: A comma cannot be entered in the percentage input field
Go to staging.new.expensify.com
Go to workspace chat.
Create an expense of the amount 30.00
Open expense report.
Click More > Split > Percent.
Click on any input field.
Enter a comma.
Verify that a comma can be entered now in the percentage input field
Bug 2: Numbers cannot be added after entering a comma
Continue from Bug 1.
Enter a number after the comma.
Verify that numbers can be entered after a comma.
Bug 3: The delete key does nothing when pressed
Continue from Bug 1.
In the Percent tab, press the Delete key.
Verify that the Delete key removes numbers from the field.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: HybridApp
Screen_Recording_20251222_004045_New.Expensify.Dev.2.mp4
Android: mWeb Chrome
Screen_Recording_20251222_002247_Brave.mp4
iOS: HybridApp
Screen.Recording.2025-12-22.at.1.13.18.AM.mov
iOS: mWeb Chrome
Screen.Recording.2025-12-22.at.1.09.24.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-19.at.9.27.04.PM.mov