-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add required
prop
#4882
Add required
prop
#4882
Conversation
🦋 Changeset detectedLatest commit: 7cc7001 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 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7cc7001:
|
Thinking about using |
test('form validation with `required` prop', () => { | ||
const components = (value?: Option) => ( | ||
<form id="formTest"> | ||
<Select {...BASIC_PROPS} required value={value || null} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing, it might be better to test that the value is explicitly passed as null instead of coalescing in case the behavior differs between undefined and null.
Not that it would here, but it's possible some future use-case changes the default behavior and it gets missed.
2c87bf5
to
758eb5c
Compare
Removed the |
998153e
to
e865624
Compare
b874afe
to
40e26f0
Compare
@@ -1992,7 +2009,7 @@ export default class Select< | |||
/> | |||
)) | |||
) : ( | |||
<input name={name} type="hidden" /> | |||
<input name={name} type="hidden" value="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, does setting value="" mean that it can't be used as an uncontrolled component anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would prevent React from throwing a "Switching from uncontrolled to controlled" error because React reuses the <input>
element for RequiredInput
component and if a value is set.
I don't know if it was really applicable here anymore, but better safe than sorry.
PS: I added a small comment regarding this in the RequiredInput
component.
@Rall3n @ebonow With this requiredInput component changes we see an accessibility issue on this new tag . I think on the below element we may need to pass the type="hidden" similar to other elements in the Select.tsx file since its has tabindex="-1".
could you please add this in the next minor release as axe dev tools (tool which is used to test the accessibility) is throwing a critical error. |
This PR adds the new
required
prop to allow for required validation in forms.This also adds a new prop
requiredMessage
to customize the content of the HTML5 validation message. I would have wanted to use a default browser message, but there is no API to get them, and the default for<input type='text'>
is imo not applicable for a dropdown component.@ebonow I don't know if and how this should be implemented into our
LiveRegion
component. Maybe you can find a solution.