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

Use NumberField instead of TextField for inputs of type number #1812

Closed
wants to merge 12 commits into from

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Nov 16, 2023

Changes

Replaces all instances of TextField where type=number with NumberField.

Background

When investigating the fix for #1438, I began investigating our ability to do transformations on text input before form submissions. react-hook-form's docs have a section on transformation and parsing in which they recommend either creating a high order controller component that receives a transform function that can operate via value/onChange or by using register props like valueAsDate, valueAsNumber orsetValueAs. The latter seems closer to what we'd actually want to do, but we're not actually using register. register hangs off of the form object returned via useForm and refactoring our inputs to use that approach would be a larger effort than seems warranted for just this functionality. The Controller component which we're using to define our components has a rules prop that takes arguments intended for register but it explicitly omits the value transformation props . I timed boxed some explorations in seeing if I could have the rules prop resolve the value transformation fields upstream but wasn't able to get to a satisfactory outcome in the time allotted so I decided just to try my hand at a user land implementation.

After that investigation I noticed we were actually doing some userland transformation in the TextField component. It's specifically around the special handling of numbers though, which seemed odd to me given that we have a dedicated NumberField component.

onChange={(e) => {
if (type === 'number') {
if (e.target.value.trim() === '') {
onChange(0)
} else if (!isNaN(e.target.valueAsNumber)) {
onChange(e.target.valueAsNumber)
}
// otherwise ignore the input. this means, for example, typing
// letters does nothing. If we instead said take anything
// that's NaN and fall back to 0, typing a letter would reset
// the field to 0, which is silly. Browsers are supposed to
// ignore non-numeric input for you anyway, but Firefox does
// not.
return
}
onChange(e.target.value)
}}

The presence of this normalization logic for numbers makes the implementation of a transformation slightly more complicated than it needs to be and (to me) points to a larger need for changes. Thus I decided to take the first step of untangling the number logic from the TextField by specifically relegating that to the NumberField

Remaining work

  • Ensure NumberField has the same input handling logic as was present in TextField
  • Remove the number transformation logic from TextField
  • Do a UI pass to ensure the changes in field components didn't cause a visual regression

Copy link

vercel bot commented Nov 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Dec 2, 2023 6:13pm

@@ -28,12 +28,13 @@ import { capitalize } from '@oxide/util'
import { ErrorMessage } from './ErrorMessage'

export interface TextFieldProps<
Type,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually like this change, it's a bit of a contagion. It's used to aid in typing the transform but I suspect there might be a better way to do this.

@zephraph
Copy link
Contributor Author

Supplanted by #1854 and #1926

@zephraph zephraph closed this Jan 31, 2024
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.

1 participant