Add regex to TextInput #51202
Merged
Add regex to TextInput #51202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@brunovjk Code-wise this is ready for review. Will add tests and screenshots later |
|
@s77rt I managed to reproduce a bug now that didn't exist before. Only on web browser, in native everything works fine.
Screen.Recording.2024-10-22.at.14.47.31.movCurious to know your test results :D Thank you very much. |
|
@brunovjk Thanks for catching that. It has been fixed and this is ready for review. |
|
Very good @s77rt, everything seems really good to me, just for one detail: If you type the maximum number of numbers, both in the integer and decimal places, and move the cursor to the point ( 51202_bug.mov |
|
@brunovjk That's expected because if you were allowed to remove the dot the entered number will exceed the maximum. |
Reviewer Checklist
Screenshots/VideosAndroid: Native51202_android_native.movAndroid: mWeb Chrome51202_android_web.moviOS: Native51202_ios_native.moviOS: mWeb Safari51202_ios_web.movMacOS: Chrome / Safari51202_web_browser.movMacOS: Desktop51202_web_desktop.mov |
Great, thanks for the explanation, it seems obvious now 😳 🤣 |
brunovjk
approved these changes
Oct 23, 2024
luacmartins
reviewed
Oct 24, 2024
src/libs/MoneyRequestUtils.ts
Outdated
| function amountRegex(decimals: number, amountMaxLength: number = CONST.IOU.AMOUNT_MAX_LENGTH): string { | ||
| return decimals === 0 | ||
| ? `^\\d{0,${amountMaxLength}}$` // Don't allow decimal point if decimals === 0 | ||
| : `^\\d{0,${amountMaxLength}}(\\.\\d{0,${decimals}})?$`; // Allow the decimal point and the desired number of digits after the point |
There was a problem hiding this comment.
How does work with internationalization where , is used instead of a .?
There was a problem hiding this comment.
Oh great catch! I think I broke that 😅 On main we replace the commas with periods but with the regex prop you won't be even allowed to insert a comma. The problem is due to the fact that what the user is allowed to enter and what we consider a valid input are two different things (because we format the user input).
i believe same bug occurs with entering .45
.45is not a valid input- but the user can still enter
.45because we format it to0.45
There was a problem hiding this comment.
I'm going to bring this back to draft to see how we can handle this. We can have 2 regexes (one for user input validation and the other for formatted text validation) but I don't really like that option. Will look for better options
There was a problem hiding this comment.
I just realized that the second problem won't occur because the modified regex already allows for .45. To keep this simple I added the comma to the regex amount so we can insert it, we still replace it with a period.
Screen.Recording.2024-10-25.at.8.36.07.PM.mov
|
Excuse me @s77rt, I got confused, can I test again? Thanks. |
|
@brunovjk Yes please do. Mostly you just need to make sure that we can type a comma and that it gets replaced by a period (as in main). |
|
First it quickly appears pr.movHowever, I can't reproduce it consistently. Can you @s77rt? I wonder if it's something with my build. Otherwise, everything seems fine to me: Screenshots/VideosAndroid: Native51202_android_native.movAndroid: mWeb Chrome51202_android_web.moviOS: Native51202_ios_native.moviOS: mWeb Safari51202_ios_web.movMacOS: Chrome / Safari51202_web_chrome.movMacOS: Desktop51202_web_desktop.mov |
|
@brunovjk That's the expected results for now |
brunovjk
approved these changes
Oct 29, 2024
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
Performance degradtion not caused by this PR. I saw other merged PRs failing with this already |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.56-0 🚀
|
50 tasks
8 tasks
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.56-9 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #47875
PROPOSAL: #47875 (comment)
Tests
45.67Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop