rework MultiSelect into controlled input; refactoring

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;

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.removeItem = this.removeItem.bind(this);
-    this.removeChip = this.removeChip.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) {
+  addItem(item) {
-    const { chipItems } = this.state;
+    const { value, onAddNewItem, onChange } = this.props;
-    const { onAddNewItem, onChange } = this.props;
+    const items = value.concat(item);
-    e.preventDefault();
-
-    const items = chipItems.concat({ name: item.name, id: item.id });
-    this.setState({
-      chipItems: items,
-      isExpanded: false,
-    });
     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) {
+  removeItem(item) {
-    const { onRemoveItem, onChange } = this.props;
+    const { value, onRemoveItem, onChange } = this.props;
-    const { chipItems } = this.state;
+    const remainingItems = value.filter(chip => chip.id !== item.id);
-    const chips = chipItems.filter(chip => chip.id !== item.id);
 
-    this.setState({ chipItems: chips });
     onRemoveItem(item);
-    onChange(chips);
+    onChange(remainingItems);
-
-    e.preventDefault();
   }
 
   render() {
-    const { options } = this.props;
+    const { value, options } = this.props;
-    const { chipItems, input, isExpanded } = this.state;
+    const { input, isExpanded } = this.state;
 
-    const list = options.map(option => (
+    const dropdownOptions = options.map(option => (
       <Fragment key={option.id}>
         {option.name.includes(input) ? (
           <DropdownItem
             component="button"
-            isDisabled={chipItems.some(item => 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 {
       </Fragment>
     ));
 
-    const chips = (
-      <ChipGroup>
-        {chipItems &&
-          chipItems.map(item => (
-            <Chip
-              key={item.id}
-              onClick={e => {
-                this.removeChip(e, item);
-              }}
-            >
-              {item.name}
-            </Chip>
-          ))}
-      </ChipGroup>
-    );
     return (
       <Fragment>
         <InputGroup>
@@ -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}
             />
             <Dropdown
               type="button"
               isPlain
-              value={chipItems}
+              value={value}
               toggle={<DropdownToggle isPlain>Labels</DropdownToggle>}
-              // 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}
             />
           </div>
-          <div css="margin: 10px">{chips}</div>
+          <div css="margin: 10px">
+            <ChipGroup>
+              {value.map(item => (
+                <Chip
+                  key={item.id}
+                  onClick={() => { this.removeItem(item); }}
+                >
+                  {item.name}
+                </Chip>
+              ))}
+            </ChipGroup>
+          </div>
         </InputGroup>
       </Fragment>
     );

awx/ui_next/src/components/MultiSelect/MultiSelect.test.jsx

@@ -1,48 +1,51 @@
 import React from 'react';
-import { mount } from 'enzyme';
+import { mount, shallow } from 'enzyme';
-import { sleep } from '@testUtils/testUtils';
 import MultiSelect from './MultiSelect';
 
 describe('<MultiSelect />', () => {
-  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', () => {
+  test('should render successfully', () => {
-    const wrapper = mount(
+    const wrapper = shallow(
       <MultiSelect
         onAddNewItem={jest.fn()}
         onRemoveItem={jest.fn()}
-        associatedItems={associatedItems}
+        value={value}
+        options={options}
+      />
+    );
+
+    expect(wrapper.find('Chip')).toHaveLength(2);
+  });
+
+  test('should add item when typed', async () => {
+    const onChange = jest.fn();
+    const onAdd = jest.fn();
+    const wrapper = mount(
+      <MultiSelect
+        onAddNewItem={onAdd}
+        onRemoveItem={jest.fn()}
+        onChange={onChange}
+        value={value}
         options={options}
       />
     );
     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 () => {
+  test('should add item when clicked from menu', () => {
-    const wrapper = mount(
-      <MultiSelect
-        onAddNewItem={jest.fn()}
-        onRemoveItem={jest.fn()}
-        associatedItems={associatedItems}
-        options={options}
-      />
-    );
-    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', () => {
     const onAddNewItem = jest.fn();
     const onChange = jest.fn();
     const wrapper = mount(
@@ -50,48 +53,44 @@ describe('<MultiSelect />', () => {
         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' });
+    expect(onAddNewItem).toHaveBeenCalledWith(options[1]);
-    component.update();
+    const newVal = onChange.mock.calls[0][0];
-    component.instance().handleAddItem(event);
+    expect(newVal).toHaveLength(3);
-    expect(component.state().chipItems.length).toBe(3);
+    expect(newVal[2]).toEqual(options[1]);
-    expect(component.state().input.length).toBe(0);
-    expect(component.state().isExpanded).toBe(false);
-    expect(onAddNewItem).toBeCalled();
-    expect(onChange).toBeCalled();
   });
 
-  test('removeChip removes chip properly', () => {
+  test('should remove item', () => {
     const onRemoveItem = jest.fn();
     const onChange = jest.fn();
-
     const wrapper = mount(
       <MultiSelect
         onAddNewItem={jest.fn()}
         onRemoveItem={onRemoveItem}
         onChange={onChange}
-        associatedItems={associatedItems}
+        value={value}
         options={options}
       />
     );
-    const event = {
+
-      preventDefault: () => {},
+    wrapper.find('Chip').at(1).invoke('onClick')();
-    };
+
-    const component = wrapper.find('MultiSelect');
+    expect(onRemoveItem).toHaveBeenCalledWith(value[1]);
-    component
+    const newVal = onChange.mock.calls[0][0];
-      .instance()
+    expect(newVal).toHaveLength(1);
-      .removeChip(event, { name: 'Foo', id: 1, organization: 1 });
+    expect(newVal).toEqual([value[0]]);
-    expect(component.state().chipItems.length).toBe(1);
-    expect(onRemoveItem).toBeCalled();
-    expect(onChange).toBeCalled();
   });
 });

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 })}
     />

awx/ui_next/src/screens/Template/shared/JobTemplateForm.jsx

@@ -326,18 +326,23 @@ class JobTemplateForm extends Component {
           />
         </FormRow>
         <FormRow>
-          <FormGroup label={i18n._(t`Labels`)} fieldId="template-labels">
+          <Field
-            <FieldTooltip
+            name="labels"
-              content={i18n._(t`Optional labels that describe this job template,
+            render={({ field }) => (
-                such as 'dev' or 'test'. Labels can be used to group and filter
+              <FormGroup label={i18n._(t`Labels`)} fieldId="template-labels">
-                job templates and completed jobs.`)}
+                <FieldTooltip
-            />
+                  content={i18n._(t`Optional labels that describe this job template,
-            <LabelSelect
+                    such as 'dev' or 'test'. Labels can be used to group and filter
-              initialValues={template.summary_fields.labels.results}
+                    job templates and completed jobs.`)}
-              onChange={labels => setFieldValue('labels', labels)}
+                  />
-              onError={err => this.setState({ contentError: err })}
+                  <LabelSelect
-            />
+                    value={field.value}
-          </FormGroup>
+                    onChange={labels => setFieldValue('labels', labels)}
+                    onError={err => this.setState({ contentError: err })}
+                  />
+                </FormGroup>
+            )}
+          />
         </FormRow>
         <AdvancedFieldsWrapper label="Advanced">
           <FormRow>

awx/ui_next/src/screens/Template/shared/JobTemplateForm.test.jsx

@@ -158,57 +158,4 @@ describe('<JobTemplateForm />', () => {
     wrapper.find('button[aria-label="Cancel"]').prop('onClick')();
     expect(handleCancel).toBeCalled();
   });
-
-  // TODO Move this test to <LabelSelect> tests
-  test.skip('handleNewLabel should arrange new labels properly', async () => {
-    const event = { key: 'Enter', preventDefault: () => {} };
-    const wrapper = mountWithContexts(
-      <JobTemplateForm
-        template={mockData}
-        handleSubmit={jest.fn()}
-        handleCancel={jest.fn()}
-      />
-    );
-    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 <LabelSelect> tests
-  test.skip('disassociateLabel should arrange new labels properly', async () => {
-    const wrapper = mountWithContexts(
-      <JobTemplateForm
-        template={mockData}
-        handleSubmit={jest.fn()}
-        handleCancel={jest.fn()}
-      />
-    );
-    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);
-  });
 });

awx/ui_next/src/screens/Template/shared/LabelSelect.jsx

@@ -29,13 +29,8 @@ async function loadLabelOptions(setLabels, onError) {
   }
 }
 
-function LabelSelect({
+function LabelSelect({ value, onChange, onError }) {
-  initialValues, // todo: change to value, controlled ?
-  onChange,
-  onError,
-}) {
   const [options, setOptions] = useState([]);
-  // TODO: move newLabels into a prop?
   useEffect(() => {
     loadLabelOptions(setOptions, onError);
   }, []);
@@ -43,7 +38,7 @@ function LabelSelect({
   return (
     <MultiSelect
       onChange={onChange}
-      associatedItems={initialValues}
+      value={value}
       options={options}
       createNewItem={name => ({
         id: name,
@@ -54,7 +49,7 @@ function LabelSelect({
   );
 }
 LabelSelect.propTypes = {
-  initialValues: arrayOf(
+  value: arrayOf(
     shape({
       id: number.isRequired,
       name: string.isRequired,