Input validation styles #1831
Input validation styles #1831
Conversation
🦋 Changeset detectedLatest commit: 6107154 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
docs/content/TextInputWithTokens.mdx
Outdated
| <PropsTable.Row | ||
| name="hideTokenRemoveButtons" | ||
| type="boolean" | ||
| defaultValue="false" | ||
| description="Whether the remove buttons should be rendered in the tokens" | ||
| /> | ||
| <PropsTableRow name="validationStatus" type="'warning' | 'error'" description="Style the input to match the status" /> |
There was a problem hiding this comment.
If we're not removing 'warning' as part of this issue, do we need to create another one or does it already exist?
There was a problem hiding this comment.
Think you're missing 'success' from here too?
| <PropsTableRow name="validationStatus" type="'warning' | 'error'" description="Style the input to match the status" /> | |
| <PropsTableRow name="validationStatus" type="'warning' | 'error' | 'success'" description="Style the input to match the status" /> |
There was a problem hiding this comment.
Another one docs related one (sorry): Can we add an example showing validationStatus being applied?
There was a problem hiding this comment.
If we're not removing 'warning' as part of this issue, do we need to create another one or does it already exist?
We need to create one. I'm going to add this as a topic of conversation for the Primer patterns working session.
There was a problem hiding this comment.
Another one docs related one (sorry): Can we add an example showing validationStatus being applied?
Yes
| type: 'radio' | ||
| } | ||
| }, | ||
| inputSize: { |
There was a problem hiding this comment.
Heads up.. this control doesn't seem to work as intended?
Screen.Recording.2022-02-03.at.11.25.04.mov
There was a problem hiding this comment.
Not sure what happened there. Will fix.
There was a problem hiding this comment.
The size difference isn't noticeable with most token sizes unless you have text in the input.
Maybe we should get rid of the inputSize prop altogether, and rely just on the size prop. Then we can manually pass the size to the TextInputWrapper depending on what was passed to size.
I'll push something up and you can let me know what you think.
There was a problem hiding this comment.
Before:
Kapture.2022-02-08.at.18.51.03.mp4
After:
Kapture.2022-02-08.at.19.03.42.mp4
There was a problem hiding this comment.
Yep, I think this is a big improvement 👍 . Thanks for making this change.
On a side note, are there usage guidelines for this particular prop? Curious why / where you'd need some of these larger sizes. Could we be encouraging an anti-pattern?
There was a problem hiding this comment.
I think we would mostly use the larger sizes because they have a larger touch target. You would use different sizes depending on where you're rendering the input.
src/TextInputWithTokens.tsx
Outdated
| /** | ||
| * The size of the text input | ||
| */ | ||
| inputSize?: StyledWrapperProps['size'] |
There was a problem hiding this comment.
Curious about this one, could you provide some context on it please? I'm a bit confused about relationship to native size attribute on <input, which is a number? When I tried small or large it doesn't seem to make any noticeable difference either.
There was a problem hiding this comment.
Also think that given the component it named as a **TextInput**WithTokens, size prop should perhaps relate to input instead of tokens
There was a problem hiding this comment.
I agree, size should refer to the input. When this component was made, we used variant for size.
I'll change it and update size to be tokenSize.
There was a problem hiding this comment.
This won't be an issue if we just remove inputSize as I'm recommending in #1831 (comment)
| } | ||
| }, | ||
| validationStatus: { | ||
| options: ['warning', 'error', undefined], |
There was a problem hiding this comment.
We don't use 'success' for TextInput, so I didn't add it for TextInputWithTokens.
There was a problem hiding this comment.
Does it use TextInputWrapper under the hood? If so, it does support success now in the base component:
react/src/_TextInputWrapper.tsx
Line 46 in ce7bef3
There was a problem hiding this comment.
Ok, then let's accept "success" in both TextInput and TextInputWithTokens.
We're going to be discussing the "warning" and "success" statuses in next week's Primer patterns working session, so we can update if we need to.
Co-authored-by: Rez <rezrah@github.com>
Co-authored-by: Rez <rezrah@github.com>
These changes render validation styles on form inputs that didn't previously have them, and shows those validation styles in Storybook.
Closes https://github.com/github/primer/issues/495
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.