From da149d931cace2dacbbe09f16eead96fa509c6d2 Mon Sep 17 00:00:00 2001 From: Keith Grant Date: Fri, 27 Sep 2019 15:04:09 -0700 Subject: [PATCH] rework MultiSelect into controlled input; refactoring --- awx/ui_next/src/components/Chip/Chip.jsx | 1 + .../components/MultiSelect/MultiSelect.jsx | 156 ++++++++---------- .../MultiSelect/MultiSelect.test.jsx | 99 ++++++----- .../components/MultiSelect/TagMultiSelect.jsx | 2 +- .../Template/shared/JobTemplateForm.jsx | 29 ++-- .../Template/shared/JobTemplateForm.test.jsx | 53 ------ .../screens/Template/shared/LabelSelect.jsx | 11 +- 7 files changed, 139 insertions(+), 212 deletions(-) diff --git a/awx/ui_next/src/components/Chip/Chip.jsx b/awx/ui_next/src/components/Chip/Chip.jsx index f8837812f0..8c86115919 100644 --- a/awx/ui_next/src/components/Chip/Chip.jsx +++ b/awx/ui_next/src/components/Chip/Chip.jsx @@ -1,6 +1,7 @@ import { Chip } from '@patternfly/react-core'; import styled from 'styled-components'; +Chip.displayName = 'PFChip'; export default styled(Chip)` --pf-c-chip--m-read-only--PaddingTop: 3px; --pf-c-chip--m-read-only--PaddingRight: 8px; diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx index 8f6752111f..b4676dc513 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.jsx @@ -45,7 +45,7 @@ const Item = shape({ class MultiSelect extends Component { static propTypes = { - associatedItems: arrayOf(Item).isRequired, + value: arrayOf(Item).isRequired, options: arrayOf(Item), onAddNewItem: func, onRemoveItem: func, @@ -65,13 +65,11 @@ class MultiSelect extends Component { super(props); this.state = { input: '', - chipItems: this.getInitialChipItems(), isExpanded: false, }; - this.handleAddItem = this.handleAddItem.bind(this); + this.handleKeyDown = this.handleKeyDown.bind(this); this.handleInputChange = this.handleInputChange.bind(this); - this.handleSelection = this.handleSelection.bind(this); - this.removeChip = this.removeChip.bind(this); + this.removeItem = this.removeItem.bind(this); this.handleClick = this.handleClick.bind(this); this.createNewItem = this.createNewItem.bind(this); } @@ -84,33 +82,57 @@ class MultiSelect extends Component { document.removeEventListener('mousedown', this.handleClick, false); } - getInitialChipItems() { - const { associatedItems } = this.props; - return associatedItems.map(item => ({ ...item })); - } - handleClick(e, option) { if (this.node && this.node.contains(e.target)) { if (option) { - this.handleSelection(e, option); + e.preventDefault(); + this.addItem(option); } } else { this.setState({ input: '', isExpanded: false }); } } - handleSelection(e, item) { - const { chipItems } = this.state; - const { onAddNewItem, onChange } = this.props; - e.preventDefault(); - - const items = chipItems.concat({ name: item.name, id: item.id }); - this.setState({ - chipItems: items, - isExpanded: false, - }); + addItem(item) { + const { value, onAddNewItem, onChange } = this.props; + const items = value.concat(item); onAddNewItem(item); onChange(items); + this.close(); + } + + // TODO: UpArrow & DownArrow for menu navigation + handleKeyDown(event) { + const { value, options } = this.props; + const { input } = this.state; + if (event.key === 'Tab') { + this.close(); + return; + } + if (!input || event.key !== 'Enter') { + return; + } + + const isAlreadySelected = value.some(i => i.name === input); + if (isAlreadySelected) { + event.preventDefault(); + this.close(); + return; + } + + const match = options.find(item => item.name === input); + const isNewItem = !match || !value.find(item => item.id === match.id); + if (isNewItem) { + event.preventDefault(); + this.addItem(match || this.createNewItem(input)); + } + } + + close() { + this.setState({ + isExpanded: false, + input: '', + }); } createNewItem(name) { @@ -124,66 +146,28 @@ class MultiSelect extends Component { }; } - handleAddItem(event) { - const { input, chipItems } = this.state; - const { options, onAddNewItem, onChange } = this.props; - const match = options.find(item => item.name === input); - const isIncluded = chipItems.some(chipItem => chipItem.name === input); - - if (!input) { - return; - } - - if (isIncluded) { - // This event.preventDefault prevents the form from submitting - // if the user tries to create 2 chips of the same name - event.preventDefault(); - this.setState({ input: '', isExpanded: false }); - return; - } - const isNewItem = !match || !chipItems.find(item => item.id === match.id); - if (event.key === 'Enter' && isNewItem) { - event.preventDefault(); - const newItem = match || this.createNewItem(input); - const items = chipItems.concat(newItem); - this.setState({ - chipItems: items, - isExpanded: false, - input: '', - }); - onAddNewItem(newItem); - onChange(items); - } else if (!isNewItem || event.key === 'Tab') { - this.setState({ isExpanded: false, input: '' }); - } - } - handleInputChange(value) { this.setState({ input: value, isExpanded: true }); } - removeChip(e, item) { - const { onRemoveItem, onChange } = this.props; - const { chipItems } = this.state; - const chips = chipItems.filter(chip => chip.id !== item.id); + removeItem(item) { + const { value, onRemoveItem, onChange } = this.props; + const remainingItems = value.filter(chip => chip.id !== item.id); - this.setState({ chipItems: chips }); onRemoveItem(item); - onChange(chips); - - e.preventDefault(); + onChange(remainingItems); } render() { - const { options } = this.props; - const { chipItems, input, isExpanded } = this.state; + const { value, options } = this.props; + const { input, isExpanded } = this.state; - const list = options.map(option => ( + const dropdownOptions = options.map(option => ( {option.name.includes(input) ? ( item.id === option.id)} + isDisabled={value.some(item => item.id === option.id)} value={option.name} onClick={e => { this.handleClick(e, option); @@ -195,21 +179,6 @@ class MultiSelect extends Component { )); - const chips = ( - - {chipItems && - chipItems.map(item => ( - { - this.removeChip(e, item); - }} - > - {item.name} - - ))} - - ); return ( @@ -222,21 +191,32 @@ class MultiSelect extends Component { type="text" aria-label="labels" value={input} - onClick={() => this.setState({ isExpanded: true })} + onFocus={() => this.setState({ isExpanded: true })} onChange={this.handleInputChange} - onKeyDown={this.handleAddItem} + onKeyDown={this.handleKeyDown} /> Labels} - // Above is not rendered but is a required prop from Patternfly + // Above is not visible but is a required prop from Patternfly isOpen={isExpanded} - dropdownItems={list} + dropdownItems={dropdownOptions} /> -
{chips}
+
+ + {value.map(item => ( + { this.removeItem(item); }} + > + {item.name} + + ))} + +
); diff --git a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx index e52e56b3c1..77c2beae1b 100644 --- a/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx +++ b/awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx @@ -1,48 +1,51 @@ import React from 'react'; -import { mount } from 'enzyme'; -import { sleep } from '@testUtils/testUtils'; +import { mount, shallow } from 'enzyme'; import MultiSelect from './MultiSelect'; describe('', () => { - const associatedItems = [ + const value = [ { name: 'Foo', id: 1, organization: 1 }, { name: 'Bar', id: 2, organization: 1 }, ]; const options = [{ name: 'Angry', id: 3 }, { name: 'Potato', id: 4 }]; - test('Initially render successfully', () => { - const wrapper = mount( + test('should render successfully', () => { + const wrapper = shallow( + ); + + expect(wrapper.find('Chip')).toHaveLength(2); + }); + + test('should add item when typed', async () => { + const onChange = jest.fn(); + const onAdd = jest.fn(); + const wrapper = mount( + ); const component = wrapper.find('MultiSelect'); + const input = component.find('TextInput'); + input.invoke('onChange')('Flabadoo'); + input.simulate('keydown', { key: 'Enter' }); - expect(component.state().chipItems.length).toBe(2); + expect(onAdd.mock.calls[0][0].name).toEqual('Flabadoo'); + const newVal = onChange.mock.calls[0][0]; + expect(newVal).toHaveLength(3); + expect(newVal[2].name).toEqual('Flabadoo'); }); - test('handleSelection add item to chipItems', async () => { - const wrapper = mount( - - ); - const component = wrapper.find('MultiSelect'); - component - .find('input[aria-label="labels"]') - .simulate('keydown', { key: 'Enter' }); - component.update(); - await sleep(1); - expect(component.state().chipItems.length).toBe(2); - }); - - test('handleAddItem adds a chip only when Tab is pressed', () => { + test('should add item when clicked from menu', () => { const onAddNewItem = jest.fn(); const onChange = jest.fn(); const wrapper = mount( @@ -50,48 +53,44 @@ describe('', () => { onAddNewItem={onAddNewItem} onRemoveItem={jest.fn()} onChange={onChange} - associatedItems={associatedItems} + value={value} options={options} /> ); + + const input = wrapper.find('TextInput'); + input.simulate('focus'); + wrapper.update(); const event = { preventDefault: () => {}, - key: 'Enter', + target: wrapper.find('DropdownItem').at(1).getDOMNode(), }; - const component = wrapper.find('MultiSelect'); + wrapper.find('DropdownItem').at(1).invoke('onClick')(event); - component.setState({ input: 'newLabel' }); - component.update(); - component.instance().handleAddItem(event); - expect(component.state().chipItems.length).toBe(3); - expect(component.state().input.length).toBe(0); - expect(component.state().isExpanded).toBe(false); - expect(onAddNewItem).toBeCalled(); - expect(onChange).toBeCalled(); + expect(onAddNewItem).toHaveBeenCalledWith(options[1]); + const newVal = onChange.mock.calls[0][0]; + expect(newVal).toHaveLength(3); + expect(newVal[2]).toEqual(options[1]); }); - test('removeChip removes chip properly', () => { + test('should remove item', () => { const onRemoveItem = jest.fn(); const onChange = jest.fn(); - const wrapper = mount( ); - const event = { - preventDefault: () => {}, - }; - const component = wrapper.find('MultiSelect'); - component - .instance() - .removeChip(event, { name: 'Foo', id: 1, organization: 1 }); - expect(component.state().chipItems.length).toBe(1); - expect(onRemoveItem).toBeCalled(); - expect(onChange).toBeCalled(); + + wrapper.find('Chip').at(1).invoke('onClick')(); + + expect(onRemoveItem).toHaveBeenCalledWith(value[1]); + const newVal = onChange.mock.calls[0][0]; + expect(newVal).toHaveLength(1); + expect(newVal).toEqual([value[0]]); }); }); diff --git a/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx b/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx index 472327aaad..a398bc0311 100644 --- a/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx +++ b/awx/ui_next/src/components/MultiSelect/TagMultiSelect.jsx @@ -33,7 +33,7 @@ function TagMultiSelect({ onChange, value }) { setOptions(options.concat(newItem)); } }} - associatedItems={stringToArray(value)} + value={stringToArray(value)} options={options} createNewItem={name => ({ id: name, name })} /> diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx index 9a4e3d9230..d7eb506975 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx @@ -326,18 +326,23 @@ class JobTemplateForm extends Component { /> - - - setFieldValue('labels', labels)} - onError={err => this.setState({ contentError: err })} - /> - + ( + + + setFieldValue('labels', labels)} + onError={err => this.setState({ contentError: err })} + /> + + )} + /> diff --git a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx index 6825982d01..0d53e4b405 100644 --- a/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx +++ b/awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx @@ -158,57 +158,4 @@ describe('', () => { wrapper.find('button[aria-label="Cancel"]').prop('onClick')(); expect(handleCancel).toBeCalled(); }); - - // TODO Move this test to tests - test.skip('handleNewLabel should arrange new labels properly', async () => { - const event = { key: 'Enter', preventDefault: () => {} }; - const wrapper = mountWithContexts( - - ); - await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); - const multiSelect = wrapper.find( - 'FormGroup[fieldId="template-labels"] MultiSelect' - ); - const component = wrapper.find('JobTemplateForm'); - - wrapper.setState({ newLabels: [], loadedLabels: [], removedLabels: [] }); - multiSelect.setState({ input: 'Foo' }); - component - .find('FormGroup[fieldId="template-labels"] input[aria-label="labels"]') - .prop('onKeyDown')(event); - - component.instance().handleNewLabel({ name: 'Bar', id: 2 }); - const newLabels = component.state('newLabels'); - expect(newLabels).toHaveLength(2); - expect(newLabels[0].name).toEqual('Foo'); - expect(newLabels[0].organization).toEqual(1); - }); - - // TODO Move this test to tests - test.skip('disassociateLabel should arrange new labels properly', async () => { - const wrapper = mountWithContexts( - - ); - await waitForElement(wrapper, 'EmptyStateBody', el => el.length === 0); - const component = wrapper.find('JobTemplateForm'); - // This asserts that the user generated a label or clicked - // on a label option, and then changed their mind and - // removed the label. - component.instance().removeLabel({ name: 'Alex', id: 17 }); - expect(component.state().newLabels.length).toBe(0); - expect(component.state().removedLabels.length).toBe(0); - // This asserts that the user removed a label that was associated - // with the template when the template loaded. - component.instance().removeLabel({ name: 'Sushi', id: 1 }); - expect(component.state().newLabels.length).toBe(0); - expect(component.state().removedLabels.length).toBe(1); - }); }); diff --git a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx index b050a4746c..73f9b5278c 100644 --- a/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx +++ b/awx/ui_next/src/screens/Template/shared/LabelSelect.jsx @@ -29,13 +29,8 @@ async function loadLabelOptions(setLabels, onError) { } } -function LabelSelect({ - initialValues, // todo: change to value, controlled ? - onChange, - onError, -}) { +function LabelSelect({ value, onChange, onError }) { const [options, setOptions] = useState([]); - // TODO: move newLabels into a prop? useEffect(() => { loadLabelOptions(setOptions, onError); }, []); @@ -43,7 +38,7 @@ function LabelSelect({ return ( ({ id: name, @@ -54,7 +49,7 @@ function LabelSelect({ ); } LabelSelect.propTypes = { - initialValues: arrayOf( + value: arrayOf( shape({ id: number.isRequired, name: string.isRequired,