Skip to content

Commit

Permalink
[Autocomplete] Clarify value and inputValue link
Browse files Browse the repository at this point in the history
  • Loading branch information
oliviertassinari committed Apr 4, 2020
1 parent e4f66b0 commit 7242344
Show file tree
Hide file tree
Showing 17 changed files with 51 additions and 104 deletions.
9 changes: 9 additions & 0 deletions docs/src/pages/components/autocomplete/autocomplete.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ Choose one of the 248 countries.

{{"demo": "pages/components/autocomplete/CountrySelect.js"}}

### Controllable states

The component has two states that can be controlled:

1. the "value" state with the `value`/`onChange` props combination.
2. the "input value" state with the `inputValue`/`onInputChange` props combination.

> ⚠️ These two state are isolated, they should be controlled independently.
## Free solo

Set `freeSolo` to true so the textbox can contain any arbitrary value. The prop is designed to cover the primary use case of a search box with suggestions, e.g. Google search.
Expand Down
50 changes: 0 additions & 50 deletions packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1101,13 +1101,6 @@ describe('<Autocomplete />', () => {
});

describe('controlled', () => {
beforeEach(() => {
consoleWarnMock.spy();
});

afterEach(() => {
consoleWarnMock.reset();
});
it('controls the input value', () => {
const handleChange = spy();
function MyComponent() {
Expand Down Expand Up @@ -1151,49 +1144,6 @@ describe('<Autocomplete />', () => {
fireEvent.keyDown(document.activeElement, { key: 'Enter' });
expect(handleInputChange.calledBefore(handleChange)).to.equal(true);
});

it('should log warning if undefined passed as input value', () => {
function MyComponent() {
const [value, setInputValue] = React.useState('foo');
const handleInputChange = () => {
setInputValue(undefined);
};
return (
<Autocomplete
{...defaultProps}
inputValue={value}
onInputChange={handleInputChange}
renderInput={(params) => <TextField {...params} autoFocus />}
/>
);
}
render(<MyComponent />);
expect(
consoleWarnMock
.getSpy()
.calledWithExactly(
'inputValue provided was null, defaulting to empty string. Check that the inputValue property gets passed a valid string value',
),
).to.equal(true);
});
it('should treat undefined as empty string when passed in as input value', () => {
function MyComponent() {
const [value, setInputValue] = React.useState('foo');
const handleInputChange = () => {
setInputValue(undefined);
};
return (
<Autocomplete
{...defaultProps}
inputValue={value}
onInputChange={handleInputChange}
renderInput={(params) => <TextField {...params} autoFocus />}
/>
);
}
render(<MyComponent />);
expect(document.activeElement.value).to.equal('');
});
});

describe('prop: filterOptions', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui-lab/src/Pagination/usePagination.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export default function usePagination(props = {}) {
controlled: pageProp,
default: defaultPage,
name: componentName,
state: 'page',
});

const handleClick = (event, value) => {
Expand Down
2 changes: 2 additions & 0 deletions packages/material-ui-lab/src/TreeView/TreeView.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ const TreeView = React.forwardRef(function TreeView(props, ref) {
controlled: expandedProp,
default: defaultExpanded,
name: 'TreeView',
state: 'expanded',
});

const [selected, setSelectedState] = useControlled({
controlled: selectedProp,
default: defaultSelected,
name: 'TreeView',
state: 'selected',
});

/*
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui-lab/src/TreeView/TreeView.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('<TreeView />', () => {

setProps({ expanded: undefined });
expect(consoleErrorMock.messages()[0]).to.include(
'A component is changing a controlled TreeView to be uncontrolled',
'Material-UI: a component is changing the controlled expanded state of TreeView to be uncontrolled.',
);
});

Expand All @@ -59,7 +59,7 @@ describe('<TreeView />', () => {

setProps({ selected: undefined });
expect(consoleErrorMock.messages()[0]).to.include(
'A component is changing a controlled TreeView to be uncontrolled',
'Material-UI: a component is changing the controlled selected state of TreeView to be uncontrolled.',
);
});
});
Expand Down
19 changes: 6 additions & 13 deletions packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,6 @@ const defaultFilterOptions = createFilterOptions();
// Number of options to jump in list box when pageup and pagedown keys are used.
const pageSize = 5;

const getInputValue = (inputValue) => {
if (inputValue == null) {
console.warn(
'inputValue provided was null, defaulting to empty string. Check that the inputValue property gets passed a valid string value',
);
return '';
}
return inputValue;
};
export default function useAutocomplete(props) {
const {
autoComplete = false,
Expand Down Expand Up @@ -195,10 +186,12 @@ export default function useAutocomplete(props) {
default: defaultValue,
name: componentName,
});

const { current: isInputValueControlled } = React.useRef(inputValueProp != null);
const [inputValueState, setInputValue] = React.useState('');
const inputValue = isInputValueControlled ? getInputValue(inputValueProp) : inputValueState;
const [inputValue, setInputValue] = useControlled({
controlled: inputValueProp,
default: '',
name: componentName,
state: 'inputValue',
});

const [focused, setFocused] = React.useState(false);

Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/ExpansionPanel/ExpansionPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ const ExpansionPanel = React.forwardRef(function ExpansionPanel(props, ref) {
controlled: expandedProp,
default: defaultExpanded,
name: 'ExpansionPanel',
state: 'expanded',
});

const handleChange = React.useCallback(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import { assert } from 'chai';
import { assert, expect } from 'chai';
import { spy } from 'sinon';
import { createMount, getClasses, findOutermostIntrinsic } from '@material-ui/core/test-utils';
import describeConformance from '../test-utils/describeConformance';
Expand Down Expand Up @@ -193,9 +193,8 @@ describe('<ExpansionPanel />', () => {
const wrapper = mount(<ExpansionPanel expanded>{minimalChildren}</ExpansionPanel>);

wrapper.setProps({ expanded: undefined });
assert.include(
consoleErrorMock.messages()[0],
'A component is changing a controlled ExpansionPanel to be uncontrolled.',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the controlled expanded state of ExpansionPanel to be uncontrolled.',
);
});

Expand All @@ -205,7 +204,7 @@ describe('<ExpansionPanel />', () => {
wrapper.setProps({ expanded: true });
assert.include(
consoleErrorMock.messages()[0],
'A component is changing an uncontrolled ExpansionPanel to be controlled.',
'Material-UI: a component is changing the uncontrolled expanded state of ExpansionPanel to be controlled.',
);
});
});
Expand Down
10 changes: 4 additions & 6 deletions packages/material-ui/src/RadioGroup/RadioGroup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,8 @@ describe('<RadioGroup />', () => {
);

wrapper.setProps({ value: undefined });
assert.include(
consoleErrorMock.messages()[0],
'A component is changing a controlled RadioGroup to be uncontrolled.',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the controlled value state of RadioGroup to be uncontrolled.',
);
});

Expand All @@ -362,9 +361,8 @@ describe('<RadioGroup />', () => {
);

wrapper.setProps({ value: 'foo' });
assert.include(
consoleErrorMock.messages()[0],
'A component is changing an uncontrolled RadioGroup to be controlled.',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the uncontrolled value state of RadioGroup to be controlled.',
);
});
});
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/Slider/Slider.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ describe('<Slider />', () => {

setProps({ value: undefined });
expect(consoleErrorMock.messages()[0]).to.include(
'A component is changing a controlled Slider to be uncontrolled.',
'Material-UI: a component is changing the controlled value state of Slider to be uncontrolled.',
);
});

Expand All @@ -524,7 +524,7 @@ describe('<Slider />', () => {

setProps({ value: [20, 50] });
expect(consoleErrorMock.messages()[0]).to.include(
'A component is changing an uncontrolled Slider to be controlled.',
'Material-UI: a component is changing the uncontrolled value state of Slider to be controlled.',
);
});
});
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Tooltip/Tooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
controlled: openProp,
default: false,
name: 'Tooltip',
state: 'open',
});
let open = openState;

Expand Down
5 changes: 2 additions & 3 deletions packages/material-ui/src/Tooltip/Tooltip.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -522,9 +522,8 @@ describe('<Tooltip />', () => {
const wrapper = mount(<Tooltip {...defaultProps} />);

wrapper.setProps({ open: true });
assert.include(
consoleErrorMock.messages()[0],
'A component is changing an uncontrolled Tooltip to be controlled.',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the uncontrolled open state of Tooltip to be controlled.',
);
});
});
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/internal/SwitchBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const SwitchBase = React.forwardRef(function SwitchBase(props, ref) {
controlled: checkedProp,
default: Boolean(defaultChecked),
name: 'SwitchBase',
state: 'checked',
});

const muiFormControl = useFormControl();
Expand Down
8 changes: 4 additions & 4 deletions packages/material-ui/src/internal/SwitchBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,10 +374,10 @@ describe('<SwitchBase />', () => {
wrapper.setProps({ checked: true });
expect(consoleErrorMock.callCount()).to.equal(2);
expect(consoleErrorMock.messages()[0]).to.include(
'A component is changing an uncontrolled input of type checkbox to be controlled.',
'Warning: A component is changing an uncontrolled input of type checkbox to be controlled.',
);
expect(consoleErrorMock.messages()[1]).to.include(
'A component is changing an uncontrolled SwitchBase to be controlled.',
'Material-UI: a component is changing the uncontrolled checked state of SwitchBase to be controlled.',
);
}),
);
Expand All @@ -393,10 +393,10 @@ describe('<SwitchBase />', () => {
setProps({ checked: undefined });
expect(consoleErrorMock.callCount()).to.equal(2);
expect(consoleErrorMock.messages()[0]).to.include(
'A component is changing a controlled input of type checkbox to be uncontrolled.',
'Warning: A component is changing a controlled input of type checkbox to be uncontrolled.',
);
expect(consoleErrorMock.messages()[1]).to.include(
'A component is changing a controlled SwitchBase to be uncontrolled.',
'Material-UI: a component is changing the controlled checked state of SwitchBase to be uncontrolled.',
);
}),
);
Expand Down
11 changes: 6 additions & 5 deletions packages/material-ui/src/utils/useControlled.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable react-hooks/rules-of-hooks, react-hooks/exhaustive-deps */
import * as React from 'react';

export default function useControlled({ controlled, default: defaultProp, name }) {
export default function useControlled({ controlled, default: defaultProp, name, state = 'value' }) {
const { current: isControlled } = React.useRef(controlled !== undefined);
const [valueState, setValue] = React.useState(defaultProp);
const value = isControlled ? controlled : valueState;
Expand All @@ -11,12 +11,13 @@ export default function useControlled({ controlled, default: defaultProp, name }
if (isControlled !== (controlled !== undefined)) {
console.error(
[
`Material-UI: A component is changing ${
isControlled ? 'a ' : 'an un'
}controlled ${name} to be ${isControlled ? 'un' : ''}controlled.`,
`Material-UI: a component is changing the ${
isControlled ? '' : 'un'
}controlled ${state} state of ${name} to be ${isControlled ? 'un' : ''}controlled.`,
'Elements should not switch from uncontrolled to controlled (or vice versa).',
`Decide between using a controlled or uncontrolled ${name} ` +
'element for the lifetime of the component.',
"The nature of the state is determined during the first render, it's considered controlled if the value is not `undefined`.",
'More info: https://fb.me/react-controlled-components',
].join('\n'),
);
Expand All @@ -29,7 +30,7 @@ export default function useControlled({ controlled, default: defaultProp, name }
if (defaultValue !== defaultProp) {
console.error(
[
`Material-UI: A component is changing the default value of an uncontrolled ${name} after being initialized. ` +
`Material-UI: a component is changing the default ${state} state of an uncontrolled ${name} after being initialized. ` +
`To suppress this warning opt to use a controlled ${name}.`,
].join('\n'),
);
Expand Down
12 changes: 6 additions & 6 deletions packages/material-ui/src/utils/useControlled.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ describe('useControlled', () => {
expect(consoleErrorMock.callCount()).to.equal(0);
setProps({ value: 'foobar' });
expect(consoleErrorMock.callCount()).to.equal(1);
expect(consoleErrorMock.messages()[0]).to.contains(
'A component is changing an uncontrolled TestComponent to be controlled.',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the uncontrolled value state of TestComponent to be controlled.',
);
});

Expand All @@ -69,8 +69,8 @@ describe('useControlled', () => {
expect(consoleErrorMock.callCount()).to.equal(0);
setProps({ value: undefined });
expect(consoleErrorMock.callCount()).to.equal(1);
expect(consoleErrorMock.messages()[0]).to.contains(
'A component is changing a controlled TestComponent to be uncontrolled.',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the controlled value state of TestComponent to be uncontrolled.',
);
});

Expand All @@ -79,8 +79,8 @@ describe('useControlled', () => {
expect(consoleErrorMock.callCount()).to.equal(0);
setProps({ defaultValue: 1 });
expect(consoleErrorMock.callCount()).to.equal(1);
expect(consoleErrorMock.messages()[0]).to.contains(
'A component is changing the default value of an uncontrolled TestComponent after being initialized. ',
expect(consoleErrorMock.messages()[0]).to.include(
'Material-UI: a component is changing the default value state of an uncontrolled TestComponent after being initialized.',
);
});
});
8 changes: 0 additions & 8 deletions test/utils/consoleErrorMock.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,6 @@ export class ConsoleMock {
throw new Error('Requested call count before spy() was called');
};

getSpy = () => {
if (this.consoleErrorContainer) {
return console[this.methodName];
}

throw new Error('Tried to get spy before spy() was called');
};

args = () => {
throw new TypeError(
'args() was removed in favor of messages(). ' +
Expand Down

0 comments on commit 7242344

Please sign in to comment.