feat(TextInputGroup): added validation support #10815
Conversation
thatblindgeye
requested review from
a team,
mcoker,
tlabaj and
wise-king-sullyman
and removed request for
a team
|
Preview: https://patternfly-react-pr-10815.surge.sh A11y report: https://patternfly-react-pr-10815-a11y.surge.sh |
| /** Flag to indicate the toggle has no border or background */ | ||
| isPlain?: boolean; | ||
| /** Status variant of the text input group. */ | ||
| status?: 'success' | 'warning' | 'error'; |
There was a problem hiding this comment.
Should we have a 'default' value users can pass similar to <TextInput>?
There was a problem hiding this comment.
We could, though we would need to decide if we really want to include "default" as an option for a status prop; FormControlIcon and MenuToggle only have these 3 (MenuToggle has danger instead of error, which is another thing we need to align on).
There was a problem hiding this comment.
I noticed we have "default" as a value for <TextInput>
FWIW I asked about a case for that value at some point, and I think it was to provide an easy value to pass to basically unset the prop (like when the input no longer needs a status because it's good or whatever) instead of having to either conditionally rendering the prop or passing undefined or something.
There was a problem hiding this comment.
It does introduce a potential namespace conflict if there is a "default" variation of something. Our "custom" status used to be called "default" and one of the reasons we changed it was because of the naming conflict it created.
There was a problem hiding this comment.
Might also bring up the question of whether the prop in TIG should be validated to match, or if TextInput should be status.
In either case, a value of "none" might be an alternative to "default", if we wanted a means to 'unset' the prop that wasn't undefined/conditionally rendering it.
There was a problem hiding this comment.
I think status is what we use for stuff like this more often.
One thought, should the example be called with status instead of with validation if we stick with the status name?
There was a problem hiding this comment.
FYI created #10851 as a followup to cover a bulk of this. Did update the example name here, though.
There was a problem hiding this comment.
hmmm.... should we keep the name same as text input for now and update it with #10851?
Or we could deprecate "validated" prop and add the status prop.
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LPTM! Left one question, not a blocker or anything.
thatblindgeye
force-pushed
the
iss10720_tigValidation
branch
from
300cecd to
bc1d5b2
Compare
|
|
||
| export const TextInputGroupWithStatus: React.FunctionComponent = () => { | ||
| const [successValue, setSuccessValue] = React.useState('Success validation'); | ||
| const [warningValue, setWarningValue] = React.useState('Warning validation with icon at start'); |
There was a problem hiding this comment.
Quick question about "with icon at start", is the icon meant to come before the input? It is currently showing after the input on the right for both the warning and error. It doesn't look like there's support for icon position in the code but the text here kind of implies that.
There was a problem hiding this comment.
Ah so the "icon at start:" is in reference to the search icon being at the start. I could update the verbiage to say something like, "...with custom non-status icon at start..." wdyt?
thatblindgeye
force-pushed
the
iss10720_tigValidation
branch
from
bc1d5b2 to
85e96bb
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
6 participants
What: Closes #10720
Additional issues: