-
Couldn't load subscription status.
- Fork 1.3k
feat: fuzzier number matching for NumberParser #8592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
# Conflicts: # packages/@internationalized/number/src/NumberParser.ts # packages/@internationalized/number/test/NumberParser.test.js
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified that this fixes the linked issues and the updates to the parser logic make sense. Perhaps a bit silly of a case, but what would we expect if someone types "123, 456"? It has the group separator and a non matching symbol from a different format and thus resolves to "123.456" since the space becomes the decimal separator but perhaps someone could expect it to resolve to "123,456"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verified the fix for #6861 here, but noticed that I can't go into the negatives it seems? It seems to cycle between what I think is 1 and 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting case. I'll look into that as follow up
Good question, that is a little ambiguous, think I should fail to parse it instead? I could limit the failure to parse case to no numbers between the two symbols. |
|
Yeah I think maybe err on the side of caution and fail to parse it if there aren't any numbers between the two symbols |
|
Build successful! 🎉 |
This reverts commit f459439.
|
Build successful! 🎉 |
This reverts commit 6495950.
| await user.paste('1,024'); | ||
| await user.tab(); | ||
| expect(input).toHaveAttribute('value', ''); | ||
| // TODO: both of the above should parse to 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Note: For this PR at least current handling in other locales will now behave the same as en-US
// But we should fix this in a follow up
|
Build successful! 🎉 |
|
Build successful! 🎉 |
Closes #7015, Closes #3862, Closes #6861, Closes #5927, Closes #9086
Finally had some thoughts on how to address some of the issues that were being tackled in #6520
✅ Pull Request Checklist:
📝 Test Instructions:
Try pasting numbers with group/decimal characters that are opposite of what the current formatting expects. Note, this is an improvement, there are some cases we can reasonably handle, then there are those which are still ambiguous.
For example, if there are two symbols in use, one is before the other, it's reasonable to assume that the first one is group and the second is decimal.
Or if there are multiple of one symbol, but only one of the other, then it's reasonable to assume that the symbol with multiple instances is the group, and the symbol with only one instance is the decimal.
On the other side, ambiguous cases are if there is only one symbol in use, we cannot reasonably know if it's a partial number with a group separator vs a decimal symbol. At least, I have determined a good way to figure this one out.
One other note, this does not extend to units/currency symbols. Those are literal and should not be fuzzy matched/removed. For example, pasting €10 into a currency formatted numberfield should not become $10 just because the formatting is for USD. They are not equivalent.
While this PR closes 9086, it just matches other locales to the current behaviour of en-US.
🧢 Your Project: