Skip to content

Commit

Permalink
feat(NumberInput): controlled upon value prop
Browse files Browse the repository at this point in the history
This change turns `<NumberInput>` to "controlled mode", if `value`
prop exists.

Refs carbon-design-system#2489.
  • Loading branch information
asudoh committed Jun 12, 2019
1 parent 2792352 commit c285221
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 14 deletions.
18 changes: 18 additions & 0 deletions packages/react/.storybook/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,14 @@ const useExternalCss =
const useStyleSourceMap =
process.env.CARBON_REACT_STORYBOOK_USE_STYLE_SOURCEMAP === 'true';

const useControlledStateWithEventListener =
process.env.CARBON_REACT_USE_CONTROLLED_STATE_WITH_EVENT_LISTENER === 'true';
const useRtl = process.env.CARBON_REACT_STORYBOOK_USE_RTL === 'true';

const replaceTable = {
useControlledStateWithEventListener,
};

const styleLoaders = [
{
loader: 'css-loader',
Expand Down Expand Up @@ -78,6 +84,18 @@ module.exports = (baseConfig, env, defaultConfig) => {
],
};

defaultConfig.module.rules.push({
test: /(\/|\\)FeatureFlags\.js$/,
loader: 'string-replace-loader',
options: {
multiple: Object.keys(replaceTable).map(key => ({
search: `export\\s+const\\s+${key}\\s*=\\s*false`,
replace: `export const ${key} = ${replaceTable[key]}`,
flags: 'i',
})),
},
});

defaultConfig.module.rules.push({
test: /-story\.jsx?$/,
loaders: [
Expand Down
48 changes: 35 additions & 13 deletions packages/react/src/components/NumberInput/NumberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ 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 requiresIfValueExists from '../../prop-types/requiresIfValueExists';
import { useControlledStateWithValue } from '../../internal/FeatureFlags';

const { prefix } = settings;

Expand All @@ -38,7 +40,7 @@ const capMax = (max, value) =>
class NumberInput extends Component {
constructor(props) {
super(props);
let value = props.value;
let value = useControlledStateWithValue ? props.defaultValue : props.value;
if (props.min || props.min === 0) {
value = Math.max(props.min, value);
}
Expand Down Expand Up @@ -83,7 +85,9 @@ class NumberInput extends Component {
* The new value is available in 'imaginaryTarget.value'
* i.e. to get the value: evt.imaginaryTarget.value
*/
onChange: PropTypes.func,
onChange: !useControlledStateWithValue
? PropTypes.func
: requiresIfValueExists(PropTypes.func),
/**
* Provide an optional function to be called when the up/down button is clicked
*/
Expand All @@ -92,6 +96,10 @@ class NumberInput extends Component {
* Specify how much the valus should increase/decrease upon clicking on up/down button
*/
step: PropTypes.number,
/**
* Optional starting value for uncontrolled state
*/
defaultValue: PropTypes.number,
/**
* Specify the value of the input
*/
Expand Down Expand Up @@ -135,10 +143,9 @@ class NumberInput extends Component {
hideLabel: false,
iconDescription: 'choose a number',
label: ' ',
onChange: () => {},
onClick: () => {},
step: 1,
value: 0,
defaultValue: 0,
value: useControlledStateWithValue ? undefined : 0,
invalid: false,
invalidText: 'Provide invalidText',
ariaLabel: 'Numeric input field with increment and decrement buttons',
Expand All @@ -156,7 +163,7 @@ class NumberInput extends Component {

static getDerivedStateFromProps({ min, max, value }, state) {
const { prevValue } = state;
return prevValue === value
return useControlledStateWithValue || prevValue === value
? null
: {
value: capMax(max, capMin(min, value)),
Expand All @@ -165,15 +172,21 @@ class NumberInput extends Component {
}

handleChange = evt => {
if (!this.props.disabled) {
const { disabled, onChange } = this.props;
if (!disabled) {
evt.persist();
evt.imaginaryTarget = this._inputRef;
const value = evt.target.value;
this.setState(
{
value: evt.target.value,
value,
},
() => {
this.props.onChange(evt);
if (useControlledStateWithValue) {
onChange(evt, { value });
} else if (onChange) {
onChange(evt);
}
}
);
}
Expand All @@ -184,7 +197,7 @@ class NumberInput extends Component {
typeof this.state.value === 'string'
? Number(this.state.value)
: this.state.value;
const { disabled, min, max, step } = this.props;
const { disabled, min, max, step, onChange, onClick } = this.props;
const conditional =
direction === 'down'
? (min !== undefined && value > min) || min === undefined
Expand All @@ -199,8 +212,13 @@ class NumberInput extends Component {
value,
},
() => {
this.props.onClick(evt, direction);
this.props.onChange(evt, direction);
if (useControlledStateWithValue) {
onClick && onClick(evt, { value, direction });
onChange && onChange(evt, { value, direction });
} else {
onClick && onClick(evt, direction);
onChange && onChange(evt, direction);
}
}
);
}
Expand All @@ -225,6 +243,7 @@ class NumberInput extends Component {
max,
min,
step,
value,
invalid,
invalidText,
helperText,
Expand Down Expand Up @@ -254,7 +273,10 @@ class NumberInput extends Component {
min,
step,
onChange: this.handleChange,
value: this.state.value,
value:
useControlledStateWithValue && value !== undefined
? value
: this.state.value,
ariaLabel,
};

Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/internal/FeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
26 changes: 26 additions & 0 deletions packages/react/src/prop-types/requiresIfValueExists.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Copyright IBM Corp. 2016, 2018
*
* This source code is licensed under the Apache-2.0 license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* @param {Function} propType The original prop type checker.
* @returns {Function} The new prop type checker for `onChange` that makes it required if `value` exists and `readOnly` does not exist.
*/
export default function requiresIfValueExists(propType) {
return function check(props, propName, componentName, ...rest) {
const { [propName]: onChange, value, readOnly } = props;
const exists = onChange !== undefined;
const valueExists = value !== undefined;
if (__DEV__ && !exists && valueExists && !readOnly) {
return new Error(
`You provided a value prop to \`${componentName}\` 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`.'
);
}
return propType(props, propName, componentName, ...rest);
};
}

0 comments on commit c285221

Please sign in to comment.