feat(PresentationalNumberInput): introduction #2427

asudoh · 2019-05-08T23:55:05Z

<PresentationalNumberInput> is presentational-only version of <NumberInput> to let application handle its state rather than handling it internally.

Refs carbon-design-system/carbon-components-react#2207.

Changelog

New

<PresentationalNumberInput>, presentational-only version of <NumberInput> to let application handle its state rather than handling it internally.

Changed

<NumberInput> to use <PresentationalNumberInput> internally.

Testing / Reviewing

Testing should make sure our <NumberInput> is not broken.

`<PresentationalNumberInput>` is presentational-only version of `<NumberInput>` to let application handle its state rather than handling it internally. Refs carbon-design-system/carbon-components-react#2207.

emyarod

looks good to me!

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

Co-Authored-By: emyarod <[email protected]>

netlify · 2019-05-10T00:17:35Z

Deploy preview for the-carbon-components ready!

Built with commit 4fbf21d

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

netlify · 2019-05-10T00:21:09Z

Deploy preview for carbon-components-react ready!

Built with commit 39a9130

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

netlify · 2019-05-10T00:32:02Z

Deploy preview for carbon-components-react ready!

Built with commit 4fbf21d

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

joshblack

PresentationalNumberInput feels verbose to type out, I would think for controlled/uncontrolled we should try and keep the same component and change depending on stuff passed in (similar to libs like downshift)

asudoh · 2019-05-13T02:02:11Z

Leaving a note; The notion of switching controlled/uncontrolled behavior upon props has been considered, stemming from easier implementation. The notion of separate component came along as a alternative for putting precedence on separation-of-concern and clear differentiation in behavior. The controlled version can be accessed in a similar manner as filtered-version of our multi-select component.

The argument wrt verbose name may still be fair enough. Depending on users' feedback we can consider flipping the switch.

tw15egan · 2019-05-13T16:04:53Z

What are some possible alternative component names we could use?

asudoh · 2019-05-13T20:16:44Z

@tw15egan Thank you for asking! The current choice is <NumberInput.Presentational> but we can do <NumberInput.V> (from "view"), etc.

shixiedesign · 2019-05-13T21:17:52Z

Is this something like a read-only number input? We have on-going work for read-only text input. See #2177

Another question, is there an issue related to this with more use case explanations? Thanks!

asudoh · 2019-05-13T21:19:29Z

@shixiedesign "View" is as-in traditional MVC (technical pattern). Sorry for the confusion 🙇

asudoh · 2019-05-14T08:56:11Z

Created Redux Form example as discussed internally: https://codesandbox.io/s/github/asudoh/carbon-components/tree/redux-form-example/packages/react/examples/redux-form

https://github.com/asudoh/carbon-components/blob/a66a284/packages/react/examples/redux-form/src/FieldLevelValidationForm.js#L25-L43 shows the gist of it.

emyarod · 2019-05-14T13:44:50Z

@asudoh should I rename and also create an example in #2655?

joshblack · 2019-05-14T15:56:58Z

I'd be a little worried about introducing the view nomenclature here, particularly because in the traditional MVC sense the view most often represents a model and in our case we're just talking about stateless render method/functions.

I think if we back up to first principles here, and take a look at where ideas of controlled/uncontrolled originate from, we can look at the Forms documentation on the React website: https://reactjs.org/docs/forms.html

In these cases, the component itself is input. From their example:

<form>
  <label>
    Name:
    <input type="text" name="name" />
  </label>
  <input type="submit" value="Submit" />
</form>

This form has the default HTML form behavior of browsing to a new page when the user submits the form. If you want this behavior in React, it just works.

The way we dip into controlled mode is by passing in value and onChange props. For example:

  render() {
    return (
      <form onSubmit={this.handleSubmit}>
        <label>
          Name:
          <input type="text" value={this.state.value} onChange={this.handleChange} />
        </label>
        <input type="submit" value="Submit" />
      </form>
    );
  }

Our component stays the same (<input>) but now we are able to control internal state.

I think this would be a good thing to keep in mind with these decisions, and the closer we can get to this behavior the better 👍 If the goal here is to pull out stateful logic, then this would be a great opportunity to look at hooks as a natural way of separating out this logic to be re-used between components.

vpicone · 2019-05-14T16:00:04Z

Definitely agree, we need to be extremely consistent in our extraction, hooks are a great opportunity to share that logic.

joshblack · 2019-05-14T16:47:58Z

Interesting pattern from Dan that is basically what we were talking about yesterday in our Slack thread: https://twitter.com/dan_abramov/status/1065354652697444353

vpicone · 2019-05-14T20:00:33Z

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

+  label: ' ',
+  onChange: () => {},
+  onClick: () => {},
+  step: 1,


Is the step prop here relevant?

AFAIK it's for determining how many number increases by up/down arrow

vpicone · 2019-05-14T20:00:50Z

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

+  ariaLabel: 'Numeric input field with increment and decrement buttons',
+  helperText: '',
+  light: false,
+  allowEmpty: false,


Should this be handled with invalid?

Seems to have come from a contributor (#1352). invalid explicitly makes the UI show the invalid state, whereas allowEmpty does internal validation for user-entered empty string.

vpicone · 2019-05-14T20:03:04Z

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

+  invalidText: 'Provide invalidText',
+  ariaLabel: 'Numeric input field with increment and decrement buttons',
+  helperText: '',
+  light: false,


As long as we're making a new API can we switch to a variant approach and away from boolean props? Something like variant="light" to allow for future variations with props that don't conflict.

vpicone · 2019-05-14T20:04:18Z

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

@@ -5,5 +5,10 @@
 * LICENSE file in the root directory of this source tree.
 */

+import NumberInput, { NumberInputView } from './NumberInput';
+
+NumberInput.View = NumberInputView;


While there's no inherent problem in also tacking it on here, I think we need to pick a usage method for our documentation and stick with it.

vpicone · 2019-05-14T20:08:22Z

As far as naming is concerned, I think we can convey the distinction between the components better by sticking to react jargon.

maybe NumberInput.Controlled = ControlledNumberInput

vpicone · 2019-05-14T20:08:55Z

Interesting pattern from Dan that is basically what we were talking about yesterday in our Slack thread: twitter.com/dan_abramov/status/1065354652697444353

@asudoh this is looks like a really powerful pattern worth considering

asudoh · 2019-05-14T20:35:04Z

While I wonder why Redux wasn't brought up in the tweet, @joshblack's point wrt following how React handles native <input> sounds fair enough. Let me close this PR, and will come up with a newer PR with "auto-switching" approach.

feat(PresentationalNumberInput): introduction

e46f635

`<PresentationalNumberInput>` is presentational-only version of `<NumberInput>` to let application handle its state rather than handling it internally. Refs carbon-design-system/carbon-components-react#2207.

asudoh mentioned this pull request May 8, 2019

feat(PresentationalNumberInput): introduction carbon-design-system/carbon-components-react#2333

Closed

asudoh requested a review from emyarod May 8, 2019 23:55

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

emyarod approved these changes May 9, 2019

View reviewed changes

Merge branch 'master' into presentational-number-input

a4783aa

emyarod reviewed May 9, 2019

View reviewed changes

packages/react/src/components/NumberInput/NumberInput.js Outdated Show resolved Hide resolved

emyarod added the status: one more review 👀 label May 9, 2019

emyarod mentioned this pull request May 9, 2019

refactor(PasswordInput): use stateless pw input #2655

Closed

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

39a9130

Co-Authored-By: emyarod <[email protected]>

Merge branch 'master' into presentational-number-input

54f9825

joshblack reviewed May 11, 2019

View reviewed changes

Merge branch 'master' into presentational-number-input

2100cb3

Merge branch 'master' into presentational-number-input

7fe3042

tw15egan and others added 2 commits May 13, 2019 14:40

Merge branch 'master' into presentational-number-input

8006a40

chore(NumberInput): decorate/normalize onChange

ae9daa6

chore(NumberInput): rename presentational version

4fbf21d

vpicone reviewed May 14, 2019

View reviewed changes

asudoh closed this May 14, 2019

feat(PresentationalNumberInput): introduction #2427

feat(PresentationalNumberInput): introduction #2427

Uh oh!

Conversation

asudoh commented May 8, 2019

Changelog

Testing / Reviewing

Uh oh!

emyarod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

netlify bot commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented May 10, 2019

Uh oh!

netlify bot commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshblack left a comment

Choose a reason for hiding this comment

Uh oh!

asudoh commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tw15egan commented May 13, 2019

Uh oh!

asudoh commented May 13, 2019

Uh oh!

shixiedesign commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asudoh commented May 13, 2019

Uh oh!

asudoh commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emyarod commented May 14, 2019

Uh oh!

joshblack commented May 14, 2019

Uh oh!

vpicone commented May 14, 2019

Uh oh!

joshblack commented May 14, 2019

Uh oh!

vpicone May 14, 2019

Choose a reason for hiding this comment

Uh oh!

asudoh May 14, 2019

Choose a reason for hiding this comment

Uh oh!

vpicone May 14, 2019

Choose a reason for hiding this comment

Uh oh!

asudoh May 14, 2019

Choose a reason for hiding this comment

Uh oh!

vpicone May 14, 2019

Choose a reason for hiding this comment

Uh oh!

vpicone May 14, 2019

Choose a reason for hiding this comment

Uh oh!

vpicone commented May 14, 2019

Uh oh!

vpicone commented May 14, 2019

Uh oh!

asudoh commented May 14, 2019

Uh oh!

Uh oh!

netlify bot commented May 10, 2019 •

edited

Loading

netlify bot commented May 10, 2019 •

edited

Loading

asudoh commented May 13, 2019 •

edited

Loading

shixiedesign commented May 13, 2019 •

edited

Loading

asudoh commented May 14, 2019 •

edited

Loading