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.

Sign up for GitHub

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

Jump to bottom

Explore strategies for enabling fully controlled components #2489

Closed
vpicone opened this issue Apr 12, 2019 · 7 comments
Closed

Explore strategies for enabling fully controlled components #2489

vpicone opened this issue Apr 12, 2019 · 7 comments
Labels
package: react carbon-components-react status: inactive Will close if there's no further activity within a given time

Comments

@vpicone
Copy link
Contributor

vpicone commented Apr 12, 2019

Summary

Problem: We want to give developers as much control as possible over the state of their application. However, we don't want to require developers to totally manage the state of each component.

I believe we should explore patterns that allow a component to be uncontrolled by default, but permit developers to use them in a controlled fashion should they so choose.

Info on controlled & uncontrolled components.

Justification

Some components can operate simultaneously as both controlled and uncontrolled in a single instance. This causes conflicting states and buggy components behavior.

Currently, for fully controlled components, developers must use a key to force re-renders. This works, and is a recommended alternative to the issue mentioned above, however a new/random key might not always be available and using one results in a hacky developer experience.

To get around this, we've been using getDerivedStateFromProps. While this works in some instances, the implementation is often tough to reason about and can greatly complicate component logic especially if there are multiple controlled props. getDerivedStateFromProps is a static method that shouldn't care about a specific instance's state.

The React team covered our exact work around/implementation here and discussed it's buggy nature.

More info: You Probably Don't Need Derived State

Desired UX and success metrics

  • Establish a pattern for using a component in both a controlled and uncontrolled fashion.

"Must have" functionality

  • The component should be uncontrolled by default with the ability to use it in a controlled manner.
  • The component should never be both controlled and uncontrolled

Milestone: Tech Debt Q2/Q3

PR incoming
@asudoh
Copy link
Contributor

asudoh commented Apr 12, 2019

CC @emyarod great if you can work with @vpicone to discuss the approach and cover the rest of the components (may need backward-compatibility layer). Great if you can review the PR. Thanks!

@stale
Copy link

stale bot commented May 1, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. Thanks for your contributions.

@carbon-bot carbon-bot transferred this issue from carbon-design-system/carbon-components-react May 9, 2019
@carbon-bot
Copy link
Contributor

Hi there! 👋 If you're wondering why this issue was moved, we're currently updating our repo structure so that every package is found in the same project.

This should not have any impact for you, but we wanted to give you a heads up in case you were wondering what is going on. If you have any questions, feel free to reach out to us on Slack or contact us at: carbon@us.ibm.com. Thanks!

@carbon-bot carbon-bot added the package: react carbon-components-react label May 9, 2019
@emyarod emyarod mentioned this issue May 9, 2019
Closed
asudoh added a commit to asudoh/carbon-components that referenced this issue May 14, 2019
@asudoh
feat(NumberInput): controlled upon onChange prop
fd59103 
This change turns `<NumberInput>` to "controlled mode", if `onChange`
prop exists.

Refs carbon-design-system#2489.
@asudoh asudoh mentioned this issue May 14, 2019
Closed
@lovemecomputer lovemecomputer mentioned this issue May 29, 2019
Closed
52 tasks
@stale
Copy link

stale bot commented Jun 8, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

@stale stale bot added the status: inactive Will close if there's no further activity within a given time label Jun 8, 2019
@asudoh asudoh removed the status: inactive Will close if there's no further activity within a given time label Jun 9, 2019
asudoh added a commit to asudoh/carbon-components that referenced this issue Jun 12, 2019
@asudoh
feat(NumberInput): controlled upon value prop
c285221 
This change turns `<NumberInput>` to "controlled mode", if `value`
prop exists.

Refs carbon-design-system#2489.
asudoh added a commit to asudoh/carbon-components that referenced this issue Jun 12, 2019
@asudoh
feat(NumberInput): controlled upon value prop
3c04166 
This change turns `<NumberInput>` to "controlled mode", if `value`
prop exists.

Refs carbon-design-system#2489.
asudoh added a commit to asudoh/carbon-components that referenced this issue Jun 12, 2019
@asudoh
feat(NumberInput): controlled upon value prop
845433a 
This change turns `<NumberInput>` to "controlled mode", if `value`
prop exists.

Also this change introduces `readOnly` prop to `<NumberInput>` with
corresponding style.

Refs carbon-design-system#2489.

feat(NumberInput): support readOnly
asudoh added a commit to asudoh/carbon-components that referenced this issue Jun 12, 2019
@asudoh
feat(NumberInput): controlled upon value prop
c33b295 
This change turns `<NumberInput>` to "controlled mode", if `value`
prop exists. Such "controlled mode" switch is behind
`useControlledStateWithEventListener` feature flag for now to given
it's a breaking change.

Also this change introduces `readOnly` prop to `<NumberInput>` with
corresponding style.

Refs carbon-design-system#2489.
@asudoh asudoh mentioned this issue Jun 12, 2019
Merged
@zachhardesty7
Copy link
Contributor

zachhardesty7 commented Jun 14, 2019 •
edited
Loading

Ran across this issue and it seems like with the latest update to 7.3.1 include #3015 fixes a similar bug seen in #3013. You can combine using defaultValue and value to cover my use case of trying to populate fields programmatically with API data.

asudoh added a commit that referenced this issue Jun 19, 2019
@asudoh
Use controlled state with value (#3028)
64c5953 
This change turns `<NumberInput>` to "controlled mode", if `value`
prop exists. Such "controlled mode" switch is behind
`useControlledStateWithValue ` feature flag for now to given
it's a breaking change.

Also this change introduces `readOnly` prop to `<NumberInput>` with
corresponding style.

Refs #2489.
@stale
Copy link

stale bot commented Jul 14, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

@stale stale bot added the status: inactive Will close if there's no further activity within a given time label Jul 14, 2019
@stale
Copy link

stale bot commented Jul 17, 2019

As there's been no activity since this issue was marked as stale, we are auto-closing it.

@stale stale bot closed this as completed Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
package: react carbon-components-react status: inactive Will close if there's no further activity within a given time
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@asudoh @vpicone @zachhardesty7 @carbon-bot