From e46f635034aa16821f393bc72d6d0b8a9e0bc9ed Mon Sep 17 00:00:00 2001 From: Akira Sudoh Date: Thu, 9 May 2019 08:51:44 +0900 Subject: [PATCH 1/4] feat(PresentationalNumberInput): introduction `` is presentational-only version of `` to let application handle its state rather than handling it internally. Refs carbon-design-system/carbon-components-react#2207. --- .../src/components/NumberInput/NumberInput.js | 581 +++++++++++------- .../react/src/components/NumberInput/index.js | 7 +- 2 files changed, 381 insertions(+), 207 deletions(-) diff --git a/packages/react/src/components/NumberInput/NumberInput.js b/packages/react/src/components/NumberInput/NumberInput.js index 3e65b9e8ba7b..87e2f4740d07 100644 --- a/packages/react/src/components/NumberInput/NumberInput.js +++ b/packages/react/src/components/NumberInput/NumberInput.js @@ -6,13 +6,12 @@ */ import PropTypes from 'prop-types'; -import React, { Component } from 'react'; +import React, { Component, useCallback } from 'react'; import classNames from 'classnames'; import { settings } from 'carbon-components'; 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'; const { prefix } = settings; @@ -26,217 +25,33 @@ const defaultTranslations = { [translationIds['decrement.number']]: 'Decrement number', }; -const capMin = (min, value) => - isNaN(min) || (!min && min !== 0) || isNaN(value) || (!value && value !== 0) - ? value - : Math.max(min, value); -const capMax = (max, value) => - isNaN(max) || (!max && max !== 0) || isNaN(value) || (!value && value !== 0) - ? value - : Math.min(max, value); - -class NumberInput extends Component { - constructor(props) { - super(props); - let value = props.value; - if (props.min || props.min === 0) { - value = Math.max(props.min, value); - } - this.state = { value }; - } - - static propTypes = { - /** - * Specify an optional className to be applied to the wrapper node - */ - className: PropTypes.string, - /** - * Specify if the control should be disabled, or not - */ - disabled: PropTypes.bool, - /** - * Specify whether you want the underlying label to be visually hidden - */ - hideLabel: PropTypes.bool, - /** - * Provide a description for up/down icons that can be read by screen readers - */ - iconDescription: PropTypes.string.isRequired, - /** - * Specify a custom `id` for the input - */ - id: PropTypes.string.isRequired, - /** - * Generic `label` that will be used as the textual representation of what - * this field is for - */ - label: PropTypes.node, - /** - * The maximum value. - */ - max: PropTypes.number, - /** - * The minimum value. - */ - min: PropTypes.number, - /** - * The new value is available in 'imaginaryTarget.value' - * i.e. to get the value: evt.imaginaryTarget.value - */ - onChange: PropTypes.func, - /** - * Provide an optional function to be called when the up/down button is clicked - */ - onClick: PropTypes.func, - /** - * Specify how much the valus should increase/decrease upon clicking on up/down button - */ - step: PropTypes.number, - /** - * Specify the value of the input - */ - value: PropTypes.number, - /** - * Specify if the currently value is invalid. - */ - invalid: PropTypes.bool, - /** - * Message which is displayed if the value is invalid. - */ - invalidText: PropTypes.string, - /** - * Provide text that is used alongside the control label for additional help - */ - helperText: PropTypes.node, - /** - * Provide a description that would be used to best describe the use case of the NumberInput component - */ - ariaLabel: PropTypes.string, - /** - * `true` to use the light version. - */ - light: PropTypes.bool, - /** - * `true` to allow empty string. - */ - allowEmpty: PropTypes.bool, - /** - * Provide custom text for the component for each translation id - */ - translateWithId: PropTypes.func.isRequired, - /** - * `true` to use the mobile variant. - */ - isMobile: PropTypes.bool, - }; - - static defaultProps = { - disabled: false, - hideLabel: false, - iconDescription: 'choose a number', - label: ' ', - onChange: () => {}, - onClick: () => {}, - step: 1, - value: 0, - invalid: false, - invalidText: 'Provide invalidText', - ariaLabel: 'Numeric input field with increment and decrement buttons', - helperText: '', - light: false, - allowEmpty: false, - translateWithId: id => defaultTranslations[id], - }; - - /** - * The DOM node refernce to the ``. - * @type {HTMLInputElement} - */ - _inputRef = null; - - static getDerivedStateFromProps({ min, max, value }, state) { - const { prevValue } = state; - return prevValue === value - ? null - : { - value: capMax(max, capMin(min, value)), - prevValue: value, - }; - } - - handleChange = evt => { - if (!this.props.disabled) { - evt.persist(); - evt.imaginaryTarget = this._inputRef; - this.setState( - { - value: evt.target.value, - }, - () => { - this.props.onChange(evt); - } - ); - } - }; - - handleArrowClick = (evt, direction) => { - let value = - typeof this.state.value === 'string' - ? Number(this.state.value) - : this.state.value; - const { disabled, min, max, step } = this.props; - const conditional = - direction === 'down' - ? (min !== undefined && value > min) || min === undefined - : (max !== undefined && value < max) || max === undefined; - - if (!disabled && conditional) { - value = direction === 'down' ? value - step : value + step; - evt.persist(); - evt.imaginaryTarget = this._inputRef; - this.setState( - { - value, - }, - () => { - this.props.onClick(evt, direction); - this.props.onChange(evt, direction); - } - ); - } - }; - - /** - * Preserves the DOM node ref of ``. - * @param {HTMLInputElement} ref The DOM node ref of ``. - */ - _handleInputRef = ref => { - this._inputRef = ref; - }; - - render() { - const { +export const PresentationalNumberInput = React.forwardRef( + function PresentationalNumberInput( + { className, disabled, - iconDescription, // eslint-disable-line + iconDescription, // eslint-disable-line no-unused-vars id, hideLabel, label, max, min, step, + value, invalid, invalidText, helperText, ariaLabel, light, allowEmpty, - innerRef: ref, translateWithId: t, isMobile, + onClick, + onChange, ...other - } = this.props; - + }, + ref + ) { const numberInputClasses = classNames( `${prefix}--number ${prefix}--number--helpertext`, className, @@ -253,9 +68,10 @@ class NumberInput extends Component { max, min, step, - onChange: this.handleChange, - value: this.state.value, - ariaLabel, + onClick, + onChange, + value, + 'aria-label': ariaLabel, }; const buttonProps = { @@ -265,7 +81,7 @@ class NumberInput extends Component { const inputWrapperProps = {}; let error = null; - if (invalid || (!allowEmpty && this.state.value === '')) { + if (invalid || (!allowEmpty && value === '')) { inputWrapperProps['data-invalid'] = true; error = (
{invalidText}
@@ -291,6 +107,45 @@ class NumberInput extends Component { t('decrement.number'), ]; + const handleArrowClick = useCallback( + (evt, direction) => { + if (disabled) { + return; + } + + const numericValue = typeof value === 'string' ? Number(value) : value; + const conditional = + direction === 'down' + ? (min !== undefined && numericValue > min) || min === undefined + : (max !== undefined && numericValue < max) || max === undefined; + + if (conditional) { + onClick(evt, { direction }); + onChange(evt, { + value: + direction === 'down' ? numericValue - step : numericValue + step, + direction, + inputRef: ref, + }); + } + }, + [disabled, min, max, step, value, ref, onClick, onChange] + ); + + const handleUpArrowClick = useCallback( + evt => { + handleArrowClick(evt, 'up'); + }, + [disabled, min, max, step, value, ref, onClick, onChange] + ); + + const handleDownArrowClick = useCallback( + evt => { + handleArrowClick(evt, 'down'); + }, + [disabled, min, max, step, value, ref, onClick, onChange] + ); + return (
@@ -304,7 +159,7 @@ class NumberInput extends Component {
); } +); + +PresentationalNumberInput.propTypes = { + /** + * Specify an optional className to be applied to the wrapper node + */ + className: PropTypes.string, + + /** + * Specify if the control should be disabled, or not + */ + disabled: PropTypes.bool, + + /** + * Specify whether you want the underlying label to be visually hidden + */ + hideLabel: PropTypes.bool, + + /** + * Provide a description for up/down icons that can be read by screen readers + */ + iconDescription: PropTypes.string.isRequired, + + /** + * Specify a custom `id` for the input + */ + id: PropTypes.string.isRequired, + + /** + * Generic `label` that will be used as the textual representation of what + * this field is for + */ + label: PropTypes.node, + + /** + * The maximum value. + */ + max: PropTypes.number, + + /** + * The minimum value. + */ + min: PropTypes.number, + + /** + * Provide an optional function to be called when the value is changed + */ + onChange: PropTypes.func, + + /** + * Provide an optional function to be called when the up/down button is clicked + */ + onClick: PropTypes.func, + + /** + * Specify how much the valus should increase/decrease upon clicking on up/down button + */ + step: PropTypes.number, + + /** + * Specify the value of the input + */ + value: PropTypes.number, + + /** + * Specify if the currently value is invalid. + */ + invalid: PropTypes.bool, + + /** + * Message which is displayed if the value is invalid. + */ + invalidText: PropTypes.string, + + /** + * Provide text that is used alongside the control label for additional help + */ + helperText: PropTypes.node, + + /** + * Provide a description that would be used to best describe the use case of the NumberInput component + */ + ariaLabel: PropTypes.string, + + /** + * `true` to use the light version. + */ + light: PropTypes.bool, + + /** + * `true` to allow empty string. + */ + allowEmpty: PropTypes.bool, + + /** + * Provide custom text for the component for each translation id + */ + translateWithId: PropTypes.func.isRequired, + + /** + * `true` to use the mobile variant. + */ + isMobile: PropTypes.bool, +}; + +PresentationalNumberInput.defaultProps = { + disabled: false, + hideLabel: false, + iconDescription: 'choose a number', + label: ' ', + onChange: () => {}, + onClick: () => {}, + step: 1, + value: 0, + invalid: false, + invalidText: 'Provide invalidText', + ariaLabel: 'Numeric input field with increment and decrement buttons', + helperText: '', + light: false, + allowEmpty: false, + translateWithId: id => defaultTranslations[id], +}; + +const capMin = (min, value) => + isNaN(min) || (!min && min !== 0) || isNaN(value) || (!value && value !== 0) + ? value + : Math.max(min, value); +const capMax = (max, value) => + isNaN(max) || (!max && max !== 0) || isNaN(value) || (!value && value !== 0) + ? value + : Math.min(max, value); +class NumberInput extends Component { + constructor(props) { + super(props); + let value = props.value; + if (props.min || props.min === 0) { + value = Math.max(props.min, value); + } + this.state = { value }; + } + + static propTypes = { + /** + * Specify an optional className to be applied to the wrapper node + */ + className: PropTypes.string, + /** + * Specify if the control should be disabled, or not + */ + disabled: PropTypes.bool, + /** + * Specify whether you want the underlying label to be visually hidden + */ + hideLabel: PropTypes.bool, + /** + * Provide a description for up/down icons that can be read by screen readers + */ + iconDescription: PropTypes.string.isRequired, + /** + * Specify a custom `id` for the input + */ + id: PropTypes.string.isRequired, + /** + * Generic `label` that will be used as the textual representation of what + * this field is for + */ + label: PropTypes.node, + /** + * The maximum value. + */ + max: PropTypes.number, + /** + * The minimum value. + */ + min: PropTypes.number, + /** + * The new value is available in 'imaginaryTarget.value' + * i.e. to get the value: evt.imaginaryTarget.value + */ + onChange: PropTypes.func, + /** + * Provide an optional function to be called when the up/down button is clicked + */ + onClick: PropTypes.func, + /** + * Specify how much the valus should increase/decrease upon clicking on up/down button + */ + step: PropTypes.number, + /** + * Specify the value of the input + */ + value: PropTypes.number, + /** + * Specify if the currently value is invalid. + */ + invalid: PropTypes.bool, + /** + * Message which is displayed if the value is invalid. + */ + invalidText: PropTypes.string, + /** + * Provide text that is used alongside the control label for additional help + */ + helperText: PropTypes.node, + /** + * Provide a description that would be used to best describe the use case of the NumberInput component + */ + ariaLabel: PropTypes.string, + /** + * `true` to use the light version. + */ + light: PropTypes.bool, + /** + * `true` to allow empty string. + */ + allowEmpty: PropTypes.bool, + /** + * Provide custom text for the component for each translation id + */ + translateWithId: PropTypes.func.isRequired, + /** + * `true` to use the mobile variant. + */ + isMobile: PropTypes.bool, + }; + + static defaultProps = { + disabled: false, + hideLabel: false, + iconDescription: 'choose a number', + label: ' ', + onChange: () => {}, + onClick: () => {}, + step: 1, + value: 0, + invalid: false, + invalidText: 'Provide invalidText', + ariaLabel: 'Numeric input field with increment and decrement buttons', + helperText: '', + light: false, + allowEmpty: false, + translateWithId: id => defaultTranslations[id], + }; + + static getDerivedStateFromProps({ min, max, value }, state) { + const { prevValue } = state; + return prevValue === value + ? null + : { + value: capMax(max, capMin(min, value)), + prevValue: value, + }; + } + + /** + * Handles user-initiated change in value. + * @param {Event} evt The event triggering this action. + * @param {Object} [options] The options. + * @param {number} [options.value] The new value. + * @param {string} [options.direction] + * The direction of the up/down arrow causing this change, if this action is triggered by clicking on up.down arrow. + * @param {HTMLInputEleemnt} [options.inputRef] The inner `` element representing the value. + * @private + */ + _handleChange = (evt, { value, direction, inputRef } = {}) => { + evt.persist(); + evt.imaginaryTarget = inputRef; + this.setState( + { + value, + }, + () => { + const { disabled, onClick, onChange } = this.props; + if (disabled) { + // `` takes care of preventing event being fired in disabled state. + // The code here is for simiulated testing + return; + } + if (evt.type === 'click') { + onClick(evt, direction); + onChange(evt, direction); + } else { + onChange(evt); + } + } + ); + }; + + /** + * A function pass along `onClick` only ones from ``, + * given `` fires `click` event from up/down buttons + * via ``'s `onChange` event. + * @param {Event} evt The event triggering this action. + * @private + */ + _swallowOnClick = evt => { + if (evt.target.tagName === 'INPUT') { + this.props.onClick(evt); + } + }; + + render() { + const { innerRef: ref, ...other } = this.props; + const { value } = this.state; + return ( + + ); + } } export default (() => { diff --git a/packages/react/src/components/NumberInput/index.js b/packages/react/src/components/NumberInput/index.js index 02cae5acca83..ea401f8ee08f 100644 --- a/packages/react/src/components/NumberInput/index.js +++ b/packages/react/src/components/NumberInput/index.js @@ -5,5 +5,10 @@ * LICENSE file in the root directory of this source tree. */ +import NumberInput, { PresentationalNumberInput } from './NumberInput'; + +NumberInput.Presentational = PresentationalNumberInput; + export * from './NumberInput.Skeleton'; -export default from './NumberInput'; +export { PresentationalNumberInput }; +export default NumberInput; From 39a9130ee32eae9365d4a1b21d9dba3837aa3423 Mon Sep 17 00:00:00 2001 From: Akira Sudoh Date: Fri, 10 May 2019 09:11:43 +0900 Subject: [PATCH 2/4] Update packages/react/src/components/NumberInput/NumberInput.js Co-Authored-By: emyarod --- packages/react/src/components/NumberInput/NumberInput.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/NumberInput/NumberInput.js b/packages/react/src/components/NumberInput/NumberInput.js index 87e2f4740d07..dc100b7dfa35 100644 --- a/packages/react/src/components/NumberInput/NumberInput.js +++ b/packages/react/src/components/NumberInput/NumberInput.js @@ -511,7 +511,7 @@ class NumberInput extends Component { const { disabled, onClick, onChange } = this.props; if (disabled) { // `` takes care of preventing event being fired in disabled state. - // The code here is for simiulated testing + // The code here is for simulated testing return; } if (evt.type === 'click') { From ae9daa676a13436ffeed8fdbe3e26e2f93fa5268 Mon Sep 17 00:00:00 2001 From: Akira Sudoh Date: Tue, 14 May 2019 17:52:37 +0900 Subject: [PATCH 3/4] chore(NumberInput): decorate/normalize onChange --- .../src/components/NumberInput/NumberInput.js | 58 +++++++++++-------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/packages/react/src/components/NumberInput/NumberInput.js b/packages/react/src/components/NumberInput/NumberInput.js index dc100b7dfa35..dbbc46f759c4 100644 --- a/packages/react/src/components/NumberInput/NumberInput.js +++ b/packages/react/src/components/NumberInput/NumberInput.js @@ -6,12 +6,13 @@ */ import PropTypes from 'prop-types'; -import React, { Component, useCallback } from 'react'; +import React, { Component, useCallback, useRef } from 'react'; import classNames from 'classnames'; import { settings } from 'carbon-components'; 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'; const { prefix } = settings; @@ -52,6 +53,8 @@ export const PresentationalNumberInput = React.forwardRef( }, ref ) { + const inputRef = useRef(null); + const numberInputClasses = classNames( `${prefix}--number ${prefix}--number--helpertext`, className, @@ -62,23 +65,6 @@ export const PresentationalNumberInput = React.forwardRef( } ); - const props = { - disabled, - id, - max, - min, - step, - onClick, - onChange, - value, - 'aria-label': ariaLabel, - }; - - const buttonProps = { - disabled, - type: 'button', - }; - const inputWrapperProps = {}; let error = null; if (invalid || (!allowEmpty && value === '')) { @@ -125,27 +111,51 @@ export const PresentationalNumberInput = React.forwardRef( value: direction === 'down' ? numericValue - step : numericValue + step, direction, - inputRef: ref, + inputRef: inputRef.current, }); } }, - [disabled, min, max, step, value, ref, onClick, onChange] + [disabled, min, max, step, value, onClick, onChange] ); const handleUpArrowClick = useCallback( evt => { handleArrowClick(evt, 'up'); }, - [disabled, min, max, step, value, ref, onClick, onChange] + [disabled, min, max, step, value, onClick, onChange] ); const handleDownArrowClick = useCallback( evt => { handleArrowClick(evt, 'down'); }, - [disabled, min, max, step, value, ref, onClick, onChange] + [disabled, min, max, step, value, onClick, onChange] ); + const handleChange = useCallback(evt => { + onChange(evt, { + value: evt.target.value, + inputRef: inputRef.current, + }); + }, []); + + const props = { + disabled, + id, + max, + min, + step, + onClick, + onChange: handleChange, + value, + 'aria-label': ariaLabel, + }; + + const buttonProps = { + disabled, + type: 'button', + }; + return (
@@ -171,7 +181,7 @@ export const PresentationalNumberInput = React.forwardRef( pattern="[0-9]*" {...other} {...props} - ref={ref} + ref={mergeRefs(ref, inputRef)} /> - - -
- - ); - } + return ( +
+
+ {(() => { + if (isMobile) { return ( <> {labelText} {helper}
+ - {invalid && ( - - )} -
- - -
+
); - })()} - {error} -
+ } + return ( + <> + {labelText} + {helper} +
+ + {invalid && ( + + )} +
+ + +
+
+ + ); + })()} + {error}
- ); - } -); +
+ ); +}); -PresentationalNumberInput.propTypes = { +NumberInputView.propTypes = { /** * Specify an optional className to be applied to the wrapper node */ @@ -351,7 +347,7 @@ PresentationalNumberInput.propTypes = { isMobile: PropTypes.bool, }; -PresentationalNumberInput.defaultProps = { +NumberInputView.defaultProps = { disabled: false, hideLabel: false, iconDescription: 'choose a number', @@ -520,7 +516,7 @@ class NumberInput extends Component { () => { const { disabled, onClick, onChange } = this.props; if (disabled) { - // `` takes care of preventing event being fired in disabled state. + // `` takes care of preventing event being fired in disabled state. // The code here is for simulated testing return; } @@ -537,7 +533,7 @@ class NumberInput extends Component { /** * A function pass along `onClick` only ones from ``, * given `` fires `click` event from up/down buttons - * via ``'s `onChange` event. + * via ``'s `onChange` event. * @param {Event} evt The event triggering this action. * @private */ @@ -551,7 +547,7 @@ class NumberInput extends Component { const { innerRef: ref, ...other } = this.props; const { value } = this.state; return ( -