Skip to content
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

Merged
merged 7 commits into from
Feb 28, 2024

Conversation

jantuomi
Copy link
Contributor

@jantuomi jantuomi commented Feb 26, 2024

Related:

Problem

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

  • Make the inputs type="text" to circumvent the aggressive validation, while retaining the same input experience with inputMode="decimal"
  • Replace commas with dots in an onChange handler.

This also fixes the same issue in the paidFor field inputs.

Issues with this fix

  • With 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.

@jantuomi jantuomi changed the title Fix/decimal separator validation Fix decimal separator issue in numeric form fields Feb 26, 2024
onChange={(event) => {
const value = event.target.value
const fixedValue =
typeof value === 'string'
Copy link
Contributor

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!

const value = event.target.value
const fixedValue =
typeof value === 'string'
? value.replace(/,/g, '.')
Copy link
Contributor

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!

@jantuomi
Copy link
Contributor Author

@shynst Thanks for the review! Fixed the found issues in the latest commit.

: p,
),
pattern="[0-9]+([,\.][0-9]+)?"
onChange={(event) =>
Copy link
Contributor

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

@jantuomi
Copy link
Contributor Author

@shynst Thanks. I applied those changes with the small difference of passing only the value string to enforceCurrencyPattern. Also removed a pattern field that I accidentally committed earlier.

shynst added a commit to shynst/spliit that referenced this pull request Feb 27, 2024
Copy link
Contributor

@shynst shynst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works, great!

@scastiel
Copy link
Member

💯

@scastiel scastiel merged commit 4c5f8a6 into spliit-app:main Feb 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants