-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix: support controlled and uncontrolled states in form components #2045
fix: support controlled and uncontrolled states in form components #2045
Conversation
import { useMemo } from "react"; | ||
|
||
type UseControlledStateProps<Value> = { | ||
controlledValue?: Value; // isChecked |
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.
Not sure this comment will always be accurate. We use this util in other inputs besides checkbox and radio right?
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.
Ah yea it's not relevant. I'll remove it. It's an artifact leftover from initial concept work on this.
@chrispulsinelli-okta This looks good to me. But, we should probably wait and let @KevinGhadyani-Okta have a look. Also curious how much disruption we may cause with this change |
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.
I like how explicit things are in inputUtils.ts
; agree with Bryan's comment about adjusting the comment (I think this is dev artifact)
@bryancunningham-okta there may be some cases where we break functionality where users are using the impacted components in this PR, but I feel that may be minimal. Most of the time, I suspect users will want to use these components in a controlled manner and a good portion of these changes were to support uncontrolled use cases. I do believe these components to be functioning as expected and to the spec required for controlled and uncontrolled usage. If there are non-functional changes Kevin wants (syntax, formatting, etc.) we can always address that when he's back as well. |
@chrispulsinelli-okta Yeah, that sounds good to me. Agreed that we can fast follow if need be. How are we planning to communicate any possible breaking functionality? This question is mainly for my own learning. So, if/when I do create breaking changes, I know what the process is for communicating them to the broader team. |
596c624
to
3edfdc3
Compare
Supporting controlled and uncontrolled usage of form elements
Tip
I highly recommend reviewing this PR commit by commit as I have logically broken down the changes into pieces that belong together. This is recommended as there are a lot of file changes related to the upgrade to Storybook. Fixes, features and tests are logically split by commits.
Changes summary
Select
,NativeSelect
,SearchField
,PasswordField
andTextField
Component changes
Select
Breaking change: must be declared with
defaultValue
for uncontrolled orvalue
for controlleddefaultValue
:""
for no default selected valuesstring
for value(s) to be selected by default (comma-separatedstring
ifhasMultipleValues
istrue
)undefined
otherwisevalue
:""
for no pre-selected value,[""]
for no pre-selected values (ifhasMultipleValues
istrue
)string
for selected value,string[]
for multiple pre-selected values,undefined
otherwiseNativeSelect
Breaking change: must be declared with
defaultValue
for uncontrolled orvalue
for controlledBreaking change: deprecated
isMultiSelect
to bring this in line withSelect
props and addedhasMultipleChoices
defaultValue
:""
for no default selected valuesstring
for value(s) to be selected by default (comma-separatedstring
ifhasMultipleValues
istrue
)undefined
otherwisevalue
:""
for no pre-selected value,[""]
for no pre-selected values (ifhasMultipleValues
istrue
)string
for selected value,string[]
for multiple pre-selected values,undefined
otherwiseSearchField
Breaking change: user must provide either a
defaultValue
orvalue
for when the component should be uncontrolled or controlled, respectivelydefaultValue
:""
for empty, uncontrolled inputstring
for value to be pre-populated in inputundefined
otherwisevalue
:""
for no initial controlled value,string
for initial controlled value,undefined
otherwisePasswordField
Breaking change: user must provide either a
defaultValue
orvalue
for when the component should be uncontrolled or controlled, respectivelydefaultValue
:""
for empty, uncontrolled inputstring
for value to be pre-populated in inputundefined
otherwisevalue
:""
for no initial controlled value,string
for initial controlled value,undefined
otherwiseTextField
Breaking change: user must provide either a
defaultValue
orvalue
for when the component should be uncontrolled or controlled, respectivelydefaultValue
:""
for empty, uncontrolled inputstring
for value to be pre-populated in inputundefined
otherwisevalue
:""
for no initial controlled value,string
for initial controlled value,undefined
otherwiseOKTA-669322
Summary
Testing & Screenshots