Skip to content
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

Conversation

chrispulsinelli-okta
Copy link
Contributor

@chrispulsinelli-okta chrispulsinelli-okta commented Nov 21, 2023

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

  • Adding controlled and uncontrolled support for Select, NativeSelect, SearchField, PasswordField and TextField
  • Updated stories with controlled cases and made existing stories all uncontrolled, explicitly
  • Updated types on components to support controlled and uncontrolled usage of components
  • Upgraded Storybook library to get rid of an issue that was appearing in dev tools console

Component changes

Select

Breaking change: must be declared with defaultValue for uncontrolled or value for controlled
defaultValue:

  • "" for no default selected values
  • string for value(s) to be selected by default (comma-separated string if hasMultipleValues is true)
  • undefined otherwise

value:

  • "" for no pre-selected value,
  • [""] for no pre-selected values (if hasMultipleValues is true)
  • string for selected value,
  • string[] for multiple pre-selected values,
  • undefined otherwise

NativeSelect

Breaking change: must be declared with defaultValue for uncontrolled or value for controlled
Breaking change: deprecated isMultiSelect to bring this in line with Select props and added hasMultipleChoices
defaultValue:

  • "" for no default selected values
  • string for value(s) to be selected by default (comma-separated string if hasMultipleValues is true)
  • undefined otherwise

value:

  • "" for no pre-selected value,
  • [""] for no pre-selected values (if hasMultipleValues is true)
  • string for selected value,
  • string[] for multiple pre-selected values,
  • undefined otherwise

SearchField

Breaking change: user must provide either a defaultValue or value for when the component should be uncontrolled or controlled, respectively
defaultValue:

  • "" for empty, uncontrolled input
  • string for value to be pre-populated in input
  • undefined otherwise

value:

  • "" for no initial controlled value,
  • string for initial controlled value,
  • undefined otherwise

PasswordField

Breaking change: user must provide either a defaultValue or value for when the component should be uncontrolled or controlled, respectively
defaultValue:

  • "" for empty, uncontrolled input
  • string for value to be pre-populated in input
  • undefined otherwise

value:

  • "" for no initial controlled value,
  • string for initial controlled value,
  • undefined otherwise

TextField

Breaking change: user must provide either a defaultValue or value for when the component should be uncontrolled or controlled, respectively
defaultValue:

  • "" for empty, uncontrolled input
  • string for value to be pre-populated in input
  • undefined otherwise

value:

  • "" for no initial controlled value,
  • string for initial controlled value,
  • undefined otherwise

OKTA-669322

Summary

Testing & Screenshots

  • I have confirmed this change with my designer and the Odyssey Design Team.

import { useMemo } from "react";

type UseControlledStateProps<Value> = {
controlledValue?: Value; // isChecked
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@bryancunningham-okta
Copy link
Contributor

@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

Copy link
Contributor

@benschell-okta benschell-okta left a 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)

@chrispulsinelli-okta chrispulsinelli-okta changed the title fix: support controlled and uncontrolled states for select fix: support controlled and uncontrolled states in form components Nov 29, 2023
@chrispulsinelli-okta
Copy link
Contributor Author

@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.
There were bugs though where users wanted to use it in a controlled way and the component(s) weren't fully allowing that to happen (select, for example).
There are folks waiting on these changes (especially for Select) and my preference is to not let them wait much longer, but I do understand you wanting to have Kevin also look at it.

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.

@bryancunningham-okta
Copy link
Contributor

@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.

@chrispulsinelli-okta chrispulsinelli-okta force-pushed the cp-OKTA-669322-controlled-uncontrolled-form-components branch from 596c624 to 3edfdc3 Compare December 1, 2023 19:07
@chrispulsinelli-okta chrispulsinelli-okta merged commit eacf2b4 into main Dec 1, 2023
1 check passed
@chrispulsinelli-okta chrispulsinelli-okta deleted the cp-OKTA-669322-controlled-uncontrolled-form-components branch December 1, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants