From 97c7723ccd1df6345b4a71077316d086492204e9 Mon Sep 17 00:00:00 2001 From: Trevan Richins Date: Wed, 19 Dec 2018 21:06:37 -0700 Subject: [PATCH 1/2] Keep the currently selected item selected in the dropdown in single selection mode --- CHANGELOG.md | 1 + .../__snapshots__/combo_box.test.js.snap | 140 +++++++++++++++++- src/components/combo_box/combo_box.js | 49 ++++-- src/components/combo_box/combo_box.test.js | 32 ++-- src/components/combo_box/matching_options.js | 19 ++- 5 files changed, 215 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 393f1907d70..7ed3f45e420 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## [`master`](https://github.com/elastic/eui/tree/master) - Convert the other of the services to TypeScript ([#1392](https://github.com/elastic/eui/pull/1392)) +- Changed single selection to select existing option in the list ([#1391](https://github.com/elastic/eui/pull/1391)) ## [`6.0.1`](https://github.com/elastic/eui/tree/v6.0.1) diff --git a/src/components/combo_box/__snapshots__/combo_box.test.js.snap b/src/components/combo_box/__snapshots__/combo_box.test.js.snap index bf46cbc5631..347944e9b83 100644 --- a/src/components/combo_box/__snapshots__/combo_box.test.js.snap +++ b/src/components/combo_box/__snapshots__/combo_box.test.js.snap @@ -380,7 +380,7 @@ exports[`props singleSelection is rendered 1`] = ` `; + +exports[`props singleSelection selects existing option when opened 1`] = ` +
+ + + + +
+`; diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 63f83506661..bf1b279ae70 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -63,10 +63,10 @@ export class EuiComboBox extends Component { super(props); const initialSearchValue = ''; - const { options, selectedOptions } = props; + const { options, selectedOptions, singleSelection } = props; this.state = { - matchingOptions: getMatchingOptions(options, selectedOptions, initialSearchValue, props.async), + matchingOptions: getMatchingOptions(options, selectedOptions, initialSearchValue, props.async, singleSelection), listElement: undefined, searchValue: initialSearchValue, isListOpen: false, @@ -74,6 +74,13 @@ export class EuiComboBox extends Component { activeOptionIndex: undefined, }; + // ensure that the currently selected single option is active if it is in the matchingOptions + if (singleSelection && selectedOptions.length === 1) { + if (this.state.matchingOptions.includes(selectedOptions[0])) { + this.state.activeOptionIndex = this.state.matchingOptions.indexOf(selectedOptions[0]); + } + } + this.rootId = htmlIdGenerator(); // Refs. @@ -396,8 +403,15 @@ export class EuiComboBox extends Component { onComboBoxClick = () => { // When the user clicks anywhere on the box, enter the interaction state. this.searchInput.focus(); - // If the user does this from a state in which an option has focus, then we need to clear it. - this.clearActiveOption(); + + // If the user does this from a state in which an option has focus, then we need to reset it or clear it. + if (this.props.singleSelection && this.props.selectedOptions.length === 1) { + this.setState({ + activeOptionIndex: this.state.matchingOptions.indexOf(this.props.selectedOptions[0]), + }); + } else { + this.clearActiveOption(); + } }; onOpenListClick = () => { @@ -457,18 +471,26 @@ export class EuiComboBox extends Component { } static getDerivedStateFromProps(nextProps, prevState) { - const { options, selectedOptions } = nextProps; + const { options, selectedOptions, singleSelection } = nextProps; const { searchValue } = prevState; // Calculate and cache the options which match the searchValue, because we use this information // in multiple places and it would be expensive to calculate repeatedly. - const matchingOptions = getMatchingOptions(options, selectedOptions, searchValue, nextProps.async); + const matchingOptions = getMatchingOptions(options, selectedOptions, searchValue, nextProps.async, singleSelection); + let nextActiveOptionIndex; + // ensure that the currently selected single option is active if it is in the matchingOptions + if (singleSelection && selectedOptions.length === 1) { + if (matchingOptions.includes(selectedOptions[0])) { + nextActiveOptionIndex = matchingOptions.indexOf(selectedOptions[0]); + } + } - return { matchingOptions }; + return { matchingOptions, activeOptionIndex: nextActiveOptionIndex }; } updateMatchingOptionsIfDifferent(newMatchingOptions) { const { matchingOptions } = this.state; + const { singleSelection, selectedOptions } = this.props; let areOptionsDifferent = false; @@ -485,7 +507,14 @@ export class EuiComboBox extends Component { if (areOptionsDifferent) { this.options = []; - this.setState({ matchingOptions: newMatchingOptions }); + let nextActiveOptionIndex; + // ensure that the currently selected single option is active if it is in the matchingOptions + if (singleSelection && selectedOptions.length === 1) { + if (newMatchingOptions.includes(selectedOptions[0])) { + nextActiveOptionIndex = newMatchingOptions.indexOf(selectedOptions[0]); + } + } + this.setState({ matchingOptions: newMatchingOptions, activeOptionIndex: nextActiveOptionIndex }); if (!newMatchingOptions.length) { // Prevent endless setState -> componentWillUpdate -> setState loop. @@ -497,13 +526,13 @@ export class EuiComboBox extends Component { } componentDidUpdate() { - const { options, selectedOptions } = this.props; + const { options, selectedOptions, singleSelection } = this.props; const { searchValue } = this.state; // React 16.3 has a bug (fixed in 16.4) where getDerivedStateFromProps // isn't called after a state change, and we track `searchValue` in state // instead we need to react to a change in searchValue here - this.updateMatchingOptionsIfDifferent(getMatchingOptions(options, selectedOptions, searchValue, this.props.async)); + this.updateMatchingOptionsIfDifferent(getMatchingOptions(options, selectedOptions, searchValue, this.props.async, singleSelection)); } componentWillUnmount() { diff --git a/src/components/combo_box/combo_box.test.js b/src/components/combo_box/combo_box.test.js index fceb9d55682..31092795adc 100644 --- a/src/components/combo_box/combo_box.test.js +++ b/src/components/combo_box/combo_box.test.js @@ -97,16 +97,30 @@ describe('props', () => { }); }); - test('singleSelection is rendered', () => { - const component = shallow( - - ); + describe('singleSelection', () => { + test('is rendered', () => { + const component = shallow( + + ); - expect(component).toMatchSnapshot(); + expect(component).toMatchSnapshot(); + }); + test('selects existing option when opened', () => { + const component = shallow( + + ); + + component.setState({ isListOpen: true }); + expect(component).toMatchSnapshot(); + }); }); test('isDisabled is rendered', () => { diff --git a/src/components/combo_box/matching_options.js b/src/components/combo_box/matching_options.js index 2b027a507b2..34a8241128e 100644 --- a/src/components/combo_box/matching_options.js +++ b/src/components/combo_box/matching_options.js @@ -14,10 +14,10 @@ export const getSelectedOptionForSearchValue = (searchValue, selectedOptions) => return selectedOptions.find(option => option.label.toLowerCase() === normalizedSearchValue); }; -const collectMatchingOption = (accumulator, option, selectedOptions, normalizedSearchValue, isPreFiltered) => { - // Only show options which haven't yet been selected. +const collectMatchingOption = (accumulator, option, selectedOptions, normalizedSearchValue, isPreFiltered, showPrevSelected) => { + // Only show options which haven't yet been selected unless requested. const selectedOption = getSelectedOptionForSearchValue(option.label, selectedOptions); - if (selectedOption) { + if (selectedOption && !showPrevSelected) { return false; } @@ -38,7 +38,7 @@ const collectMatchingOption = (accumulator, option, selectedOptions, normalizedS } }; -export const getMatchingOptions = (options, selectedOptions, searchValue, isPreFiltered) => { +export const getMatchingOptions = (options, selectedOptions, searchValue, isPreFiltered, showPrevSelected) => { const normalizedSearchValue = searchValue.trim().toLowerCase(); const matchingOptions = []; @@ -46,7 +46,14 @@ export const getMatchingOptions = (options, selectedOptions, searchValue, isPreF if (option.options) { const matchingOptionsForGroup = []; option.options.forEach(groupOption => { - collectMatchingOption(matchingOptionsForGroup, groupOption, selectedOptions, normalizedSearchValue, isPreFiltered); + collectMatchingOption( + matchingOptionsForGroup, + groupOption, + selectedOptions, + normalizedSearchValue, + isPreFiltered, + showPrevSelected + ); }); if (matchingOptionsForGroup.length > 0) { // Add option for group label @@ -55,7 +62,7 @@ export const getMatchingOptions = (options, selectedOptions, searchValue, isPreF matchingOptions.push(...matchingOptionsForGroup); } } else { - collectMatchingOption(matchingOptions, option, selectedOptions, normalizedSearchValue, isPreFiltered); + collectMatchingOption(matchingOptions, option, selectedOptions, normalizedSearchValue, isPreFiltered, showPrevSelected); } }); return matchingOptions; From 9f80774eeb936149f84b4b8e3120366134283a83 Mon Sep 17 00:00:00 2001 From: Trevan Richins Date: Thu, 3 Jan 2019 10:56:09 -0700 Subject: [PATCH 2/2] Ensure activeOptionIndex only changes in single selection mode --- src/components/combo_box/combo_box.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index bf1b279ae70..a51454a89be 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -477,19 +477,20 @@ export class EuiComboBox extends Component { // Calculate and cache the options which match the searchValue, because we use this information // in multiple places and it would be expensive to calculate repeatedly. const matchingOptions = getMatchingOptions(options, selectedOptions, searchValue, nextProps.async, singleSelection); - let nextActiveOptionIndex; // ensure that the currently selected single option is active if it is in the matchingOptions if (singleSelection && selectedOptions.length === 1) { + let nextActiveOptionIndex; if (matchingOptions.includes(selectedOptions[0])) { nextActiveOptionIndex = matchingOptions.indexOf(selectedOptions[0]); } + return { matchingOptions, activeOptionIndex: nextActiveOptionIndex }; } - return { matchingOptions, activeOptionIndex: nextActiveOptionIndex }; + return { matchingOptions }; } updateMatchingOptionsIfDifferent(newMatchingOptions) { - const { matchingOptions } = this.state; + const { matchingOptions, activeOptionIndex } = this.state; const { singleSelection, selectedOptions } = this.props; let areOptionsDifferent = false; @@ -507,7 +508,7 @@ export class EuiComboBox extends Component { if (areOptionsDifferent) { this.options = []; - let nextActiveOptionIndex; + let nextActiveOptionIndex = activeOptionIndex; // ensure that the currently selected single option is active if it is in the matchingOptions if (singleSelection && selectedOptions.length === 1) { if (newMatchingOptions.includes(selectedOptions[0])) {