feat(NumberInput): controlled upon onChange prop #2730

asudoh · 2019-05-14T23:12:19Z

This change turns <NumberInput> to "controlled mode", if onChange prop exists.

Refs #2489.

Changelog

New

A mechanism to turn <NumberInput> to "controlled mode", if onChange prop exists.

Testing / Reviewing

Testing should make sure <NumberInput> is not broken.

This change turns `<NumberInput>` to "controlled mode", if `onChange` prop exists. Refs carbon-design-system#2489.

asudoh · 2019-05-14T23:14:35Z

packages/react/.storybook/webpack.config.js

@@ -78,6 +84,18 @@ module.exports = (baseConfig, env, defaultConfig) => {
    ],
  };

+  defaultConfig.module.rules.push({
+    test: /(\/|\\)FeatureFlags\.js$/,


Flipping the switch upon React context is in consideration, for the time once we have finished implementing this mechanism to all form components.

asudoh · 2019-05-14T23:14:45Z

packages/react/src/components/NumberInput/NumberInput.js

@@ -13,6 +13,7 @@ import WarningFilled16 from '@carbon/icons-react/lib/warning--filled/16';
 import CaretDownGlyph from '@carbon/icons-react/lib/caret--down/index';
 import CaretUpGlyph from '@carbon/icons-react/lib/caret--up/index';
 import mergeRefs from '../../tools/mergeRefs';
+import { useControlledStateWithEventListener } from '../../internal/FeatureFlags';


Flipping the switch upon React context is in consideration, for the time once we have finished implementing this mechanism to all form components.

asudoh · 2019-05-14T23:14:56Z

packages/react/src/internal/FeatureFlags.js

@@ -17,4 +17,4 @@
 * const MyComponent = props => (<div {...props}>{aFeatureFlag ? 'foo' : 'bar'}</div>);
 */

-/* Currently no feature flag is in use, but keeping this file as a placeholder */
+export const useControlledStateWithEventListener = false;


Flipping the switch upon React context is in consideration, for the time once we have finished implementing this mechanism to all form components.

netlify · 2019-05-14T23:20:15Z

Deploy preview for the-carbon-components ready!

Built with commit fd59103

https://deploy-preview-2730--the-carbon-components.netlify.com

netlify · 2019-05-14T23:21:21Z

Deploy preview for carbon-components-react ready!

Built with commit fd59103

https://deploy-preview-2730--carbon-components-react.netlify.com

emyarod

should we expose a storybook example for the controlled component?

emyarod

looks good to me!

vpicone · 2019-05-23T15:38:30Z

@asudoh I'm not sure . Wouldn't doing so mean that inputs would need to be controlled or uncontrolled application-wide? Wrapping individual inputs or even forms in context seems like a really tedious API for developers to use.

I think we should focus on making the components themselves flexible, whether that means a distinct controlled/presentational component as we discussed previously, or the controlled mode implementation I used for the UI Shell.

Slightly off topic, but I'd also like to reiterate my aversion towards putting logic in gDSFP for the reasons outlined here: #2489

vpicone · 2019-05-23T21:34:21Z

packages/react/src/components/NumberInput/NumberInput.js

@@ -254,7 +265,10 @@ class NumberInput extends Component {
      min,
      step,
      onChange: this.handleChange,
-      value: this.state.value,
+      value:
+        useControlledStateWithEventListener && this.props.onChange


I don't think the presence of an onChange prop should be the switch that turns on controlled mode. There's many cases where developers would want to be informed 'onChange' but not control the actual value of the input themselves. Things like clearing form validation notifications, displaying computed values from inputs etc.

vpicone · 2019-05-23T22:04:34Z

My initial guess at what the a control-mode trigger would look like is: defaultValue, value, and onChange. If value is supplied, then the component is controlled.

Ideally, this is controlled using a ref so the controlled/uncontrolled nature of the component persists for it's lifecycle.

I think it would be a good pattern to establish the controlled/uncontrolled status of a component in a single location rather than using useControlledStateWithEventListener throughout.

Also, now that I finally understand what the feature flag is meant to accomplish, I think we should reconsider the name. We might want to avoid naming new things with the prefix use as this is this the naming standard for custom hooks. A more appropriate name might also leave out the already implied implementation. Maybe something like enableControlledFormInputs. Since this feature flag seems to imply global changes to component behavior, we'll definitely need to document which components are and aren't impacted.

asudoh · 2019-05-23T22:12:51Z

Thanks @vpicone for reviewing!

My initial guess at what the a control-mode trigger would look like is: defaultValue, value, and onChange. If value is supplied, then the component is controlled.

Would it be this "if all of those props are given", or "if any of those props are given"?

Maybe something like enableControlledFormInputs. Since this feature flag seems to imply global changes to component behavior, we'll definitely need to document which components are and aren't impacted.

Makes sense!

vpicone · 2019-05-24T00:31:02Z

@asudoh Sorry I didn't meant to imply any requirements there I should have been more clear. Here's better descriptions with one additional prop:

defaultValue → optional starting value for uncontrolled state
value → enables controlled mode, component ignores defaultValue in favor of this prop and will for the rest of it's lifecycle be a controlled component with this prop as it's source of truth.
onChange → optional handler. However, if value is provided and the handler is not, we'll throw a warning similar to react does for input elements:

Warning: Failed `NumberInput` propType: You provided a value prop to `NumberInput` 
without an `onChange` handler. This will render a read-only field. If the field 
should be mutable use `defaultValue`. Otherwise, set either `onChange` or `readOnly`.

This warning assumes the developer meant for the component to be uncontrolled and intended to set a default value.

readOnly → disables the prop-type warning from providing a value without an onChange handler. The developer effectively saying: "I know what I'm doing"

asudoh · 2019-05-24T02:08:15Z

Thanks @vpicone for the details! Sounds good basically, just one thing - readOnly tends to come with UX patterns, like one we are discussing for text input. That said, would you think of any other prop name? Or would it more make sense if we defer this "suppress warning" feature until we establish the UX pattern for read-only number input?

vpicone · 2019-05-24T03:38:03Z

@asudoh I haven't read that PR all the way, but while the readOnly prop acts as a 'supressWarning' it also maps to an actual form input attribute (which was mentioned in the PR). This attribute has beneficial accessibility implications when a developer is in read only mode: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#readonly

It indicates to screen readers that the input should:

receive focus, but cannot be modified by the user.
be included in the tab order.
successfully submit data.

So while additional styling might be nice eventually, its not necessary for a11y. I can't think of a more appropriate prop at the moment. This one serves the purpose of ensuring the developer has intentionally engaged 'readOnly' mode while also ensuring the read-only input is accessible.

If given the readOnly prop, the input would need to forgo all state changes. I think it's important to avoid the case where users changes aren't doing anything, which I remember was a concern of yours.

asudoh · 2019-05-24T04:35:04Z

@vpicone Great point on readonly propagated to <input>. Given you brought up this, let's discuss what happens with <input type="number" readonly>:

The former is <input type="number">, the latter is <input type="number" readonly>. I observe two things:

Grayed out border
Up/down button goes away

They are similar to <input type="number" disabled>, though readonly seems focusable whereas disabled doesn't. Our number input hasn't implemented UX styles for read-only variant.

Does it make sense? If so, and unless @carbon-design-system/design disagrees, we can apply some of styles of disabled number input style if readOnly prop is given. Thoughts? Thanks!

vpicone · 2019-05-24T13:04:27Z

@asudoh It does. Since we've determined the onChange handler might not suffice as the marker for controlled mode, a readOnly state is inevitable. Regardless, we should be supporting this native input state as close to the native input as possible I believe would address #2177 along with all the other types of input for which we have a controlled mode going forward.

It might make sense to duplicate the styles into a separate token in case we need to differentiate the styles going forward, but I agree that we should try to stick as closely to the native implementation as possible in regards to text color fading and controls disappearing.

asudoh · 2019-06-12T09:39:12Z

Superseded by: #3028

feat(NumberInput): controlled upon onChange prop

fd59103

This change turns `<NumberInput>` to "controlled mode", if `onChange` prop exists. Refs carbon-design-system#2489.

asudoh added the status: ready for review 👀 label May 14, 2019

asudoh commented May 14, 2019

View reviewed changes

emyarod reviewed May 17, 2019

View reviewed changes

asudoh added the status: one more review 👀 label May 18, 2019

emyarod approved these changes May 20, 2019

View reviewed changes

emyarod mentioned this pull request May 23, 2019

refactor(PasswordInput): use stateless pw input #2655

Closed

vpicone reviewed May 23, 2019

View reviewed changes

asudoh closed this Jun 12, 2019

asudoh deleted the use-controlled-state-with-event-listener branch June 12, 2019 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NumberInput): controlled upon onChange prop #2730

feat(NumberInput): controlled upon onChange prop #2730

asudoh commented May 14, 2019 •

edited

Loading

asudoh May 14, 2019

asudoh May 14, 2019

asudoh May 14, 2019

netlify bot commented May 14, 2019

netlify bot commented May 14, 2019

emyarod left a comment

emyarod left a comment

vpicone commented May 23, 2019

vpicone May 23, 2019

vpicone commented May 23, 2019 •

edited

Loading

asudoh commented May 23, 2019

vpicone commented May 24, 2019 •

edited

Loading

asudoh commented May 24, 2019

vpicone commented May 24, 2019 •

edited

Loading

asudoh commented May 24, 2019 •

edited

Loading

vpicone commented May 24, 2019 •

edited

Loading

asudoh commented Jun 12, 2019

feat(NumberInput): controlled upon onChange prop #2730

feat(NumberInput): controlled upon onChange prop #2730

Conversation

asudoh commented May 14, 2019 • edited Loading

Changelog

Testing / Reviewing

asudoh May 14, 2019

Choose a reason for hiding this comment

asudoh May 14, 2019

Choose a reason for hiding this comment

asudoh May 14, 2019

Choose a reason for hiding this comment

netlify bot commented May 14, 2019

netlify bot commented May 14, 2019

emyarod left a comment

Choose a reason for hiding this comment

emyarod left a comment

Choose a reason for hiding this comment

vpicone commented May 23, 2019

vpicone May 23, 2019

Choose a reason for hiding this comment

vpicone commented May 23, 2019 • edited Loading

asudoh commented May 23, 2019

vpicone commented May 24, 2019 • edited Loading

asudoh commented May 24, 2019

vpicone commented May 24, 2019 • edited Loading

asudoh commented May 24, 2019 • edited Loading

vpicone commented May 24, 2019 • edited Loading

asudoh commented Jun 12, 2019

asudoh commented May 14, 2019 •

edited

Loading

vpicone commented May 23, 2019 •

edited

Loading

vpicone commented May 24, 2019 •

edited

Loading

vpicone commented May 24, 2019 •

edited

Loading

asudoh commented May 24, 2019 •

edited

Loading

vpicone commented May 24, 2019 •

edited

Loading