-
-
Notifications
You must be signed in to change notification settings - Fork 222
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
Fix decimal separator issue in numeric form fields #115
Fix decimal separator issue in numeric form fields #115
Conversation
src/components/expense-form.tsx
Outdated
onChange={(event) => { | ||
const value = event.target.value | ||
const fixedValue = | ||
typeof value === 'string' |
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.
value is always string here!
src/components/expense-form.tsx
Outdated
const value = event.target.value | ||
const fixedValue = | ||
typeof value === 'string' | ||
? value.replace(/,/g, '.') |
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.
Since you changed the input type to "text" all types of non numeric characters are allowed here. You should consider filtering them out here!
@shynst Thanks for the review! Fixed the found issues in the latest commit. |
src/components/expense-form.tsx
Outdated
: p, | ||
), | ||
pattern="[0-9]+([,\.][0-9]+)?" | ||
onChange={(event) => |
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.
This breaks, because this field.onChange()
has a different signature than the one above.
May I suggest:
onChange={(event) =>
field.onChange(
field.value.map((p) =>
p.participant === id
? {
participant: id,
shares:
enforceCurrencyPattern(
event,
),
}
: p,
),
)
}
where enforceCurrencyPattern()
becomes
const enforceCurrencyPattern = (event: ChangeEvent<HTMLInputElement>) =>
event.target.value
// replace first comma with #
.replace(/[.,]/, '#')
// remove all other commas
.replace(/[.,]/g, '')
// change back # to dot
.replace(/#/, '.')
// remove all non-numeric and non-dot characters
.replace(/[^\d.]/g, '')
Note: this also solves the issue where text could be entered with multiple comas
@shynst Thanks. I applied those changes with the small difference of passing only the value string to |
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.
Works, great!
💯 |
Related:
amount
field #90Problem
I previously made a fix that converted commas to dots in the "amount" field during Zod validation. Turns out, on some browsers, the problem remained, and these numbers that used a comma as the decimal separator were not accepted as valid numbers.
The reason was that when using inputs with
type="number"
, browsers have different behaviour regarding validation. For example, iOS Safari rejects numbers with commas in them. This happens before the number can reach the Zod validation and be subsequently fixed with the previous fix.I first attempted to replace commas with dots in an
onChange
handler, but it turns out that the "erroneous" numeric value is not accessible to JS in that context when browser validation has deemed it invalid (I.e.event.target.value
is empty or not in sync with the inputted value). The browser validation needs to be disabled first.Fix
type="text"
to circumvent the aggressive validation, while retaining the same input experience withinputMode="decimal"
onChange
handler.This also fixes the same issue in the
paidFor
field inputs.Issues with this fix
input="text"
, desktop browsers will no longer show an "up-down" widget that nudges the value up or down. I don't think this is a big problem since not many use that anyway.