fix: pulled out TextField into separate components #1744
Conversation
KevinGhadyani-Okta
requested review from
a team,
ganeshsomasundaram-okta and
kamronbatmanghelich-okta
as code owners
|
|
||
| import { memo } from "react"; | ||
|
|
||
| import { FormHelperText, visuallyHidden } from "."; |
There was a problem hiding this comment.
Importing from "." (or index.ts, which is probably a re-export) is not recommended, especially since this is in the same folder so it is not subject to portability rules.
There was a problem hiding this comment.
We'll change these in a future PR. I'll add a ticket for it.
There was a problem hiding this comment.
KevinGhadyani-Okta
force-pushed
the
kg/OKTA-596518
branch
5 times, most recently
from
975a463 to
5f34a13
Compare
KevinGhadyani-Okta
force-pushed
the
kg/OKTA-596518
branch
from
5f34a13 to
9a8de6f
Compare
|
|
||
| const Field = ({ | ||
| errorMessage, | ||
| hasVisibleLabel, |
There was a problem hiding this comment.
This should probably default to true -- nearly all fields should have a visible label
There was a problem hiding this comment.
That's fine. It's on Field though where we're being explicit. It's internal-only. TextField, SearchField, etc won't be exposing this.
| return ( | ||
| <Field | ||
| errorMessage={errorMessage} | ||
| hasVisibleLabel |
There was a problem hiding this comment.
Maybe this should be optional if we have it default to true? Not strongly opinionated there, since we want visible labels as a standard.
There was a problem hiding this comment.
This is like hint and error. We're defining what we want on Field. Only 6-7 components will use it. Even if we expose it, this is all for dev maintenance. Someone can look at it and know what to expect. Rather than telling them to turn off a feature, they're telling this component what they want to show up.
| const FieldError = ({ id, text }: FieldErrorProps) => { | ||
| return ( | ||
| <FormHelperText error id={id}> | ||
| <span style={visuallyHidden}>Error: </span> |
There was a problem hiding this comment.
These should use ScreenReaderText now that it's available
There was a problem hiding this comment.
It has it now.
There was a problem hiding this comment.
FieldLabel does not.
It's a bit more complex:
InputLabel
htmlFor={inputId}
id={id}
style={hasVisibleLabel ? undefined : visuallyHidden}
>I'd have to duplicate this. I guess I could useCallback or useMemo it. I'll fix that now.
There was a problem hiding this comment.
This change going in before merge?
There was a problem hiding this comment.
Should be in now.
Description
This PR is a precursor to unblocking
Autocomplete.Autocompleterequires arenderfunction, and this needs to return a React component. OurTextFielddoesn't support all props needed by it, so we need to gut it and letAutocompletebuild its own input using those guts.As part of this PR:
TextField's innards have been moved toFieldsince they're shared by numerous components.PasswordFieldandSearchFieldas there's already a task to split these out.RadioGroupandCheckboxGroupis coming in a future task as they share this API.