From 53c9c8867b37c40f9171ea16eeb12f3db8d41f57 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 30 May 2018 13:13:15 -0600 Subject: [PATCH 1/5] Refactored EuiComboBox to React 16.3 lifecycle --- src/components/combo_box/combo_box.js | 156 ++++++++++++++------------ 1 file changed, 83 insertions(+), 73 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 608588716af..4bc9d174c53 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -56,19 +56,16 @@ export class EuiComboBox extends Component { const initialSearchValue = ''; const { options, selectedOptions } = props; - const matchingOptions = this.getMatchingOptions(options, selectedOptions, initialSearchValue); this.state = { + matchingOptions: getMatchingOptions(options, selectedOptions, initialSearchValue, props.async), + listBounds: undefined, searchValue: initialSearchValue, isListOpen: false, listPosition: 'bottom', activeOptionIndex: undefined, }; - // Cached derived state. - this.matchingOptions = matchingOptions; - this.listBounds = undefined; - // Refs. this.comboBox = undefined; this.autoSizeInput = undefined; @@ -77,12 +74,6 @@ export class EuiComboBox extends Component { this.options = []; } - getMatchingOptions = (options, selectedOptions, searchValue) => { - // Assume the consumer has already filtered the options against the search value. - const isPreFiltered = this.props.async; - return getMatchingOptions(options, selectedOptions, searchValue, isPreFiltered); - }; - openList = () => { this.setState({ isListOpen: true, @@ -96,7 +87,7 @@ export class EuiComboBox extends Component { }); }; - updateListPosition = (listBounds = this.listBounds) => { + updateListPosition = (listBounds = this.state.listBounds) => { if (!this._isMounted) { return; } @@ -111,26 +102,25 @@ export class EuiComboBox extends Component { const comboBoxBounds = this.comboBox.getBoundingClientRect(); - // Cache for future calls. Assign values directly instead of destructuring because listBounds is - // a DOMRect, not a JS object. - this.listBounds = { - bottom: listBounds.bottom, - height: listBounds.height, - left: comboBoxBounds.left, - right: comboBoxBounds.right, - top: listBounds.top, - width: comboBoxBounds.width, - x: listBounds.x, - y: listBounds.y, - }; - - const { position, left, top } = calculatePopoverPosition(comboBoxBounds, this.listBounds, 'bottom', 0, ['bottom', 'top']); + const { position, left, top } = calculatePopoverPosition(comboBoxBounds, listBounds, 'bottom', 0, ['bottom', 'top']); this.optionsList.style.top = `${top + window.scrollY}px`; this.optionsList.style.left = `${left}px`; this.optionsList.style.width = `${comboBoxBounds.width}px`; + // Cache for future calls. Assign values directly instead of destructuring because listBounds is + // a DOMRect, not a JS object. this.setState({ + listBounds: { + bottom: listBounds.bottom, + height: listBounds.height, + left: comboBoxBounds.left, + right: comboBoxBounds.right, + top: listBounds.top, + width: comboBoxBounds.width, + x: listBounds.x, + y: listBounds.y, + }, width: comboBoxBounds.width, listPosition: position, }); @@ -181,42 +171,42 @@ export class EuiComboBox extends Component { incrementActiveOptionIndex = throttle(amount => { // If there are no options available, reset the focus. - if (!this.matchingOptions.length) { + if (!this.state.matchingOptions.length) { this.clearActiveOption(); return; } - let nextActiveOptionIndex; + this.setState(({ activeOptionIndex, matchingOptions }) => { + let nextActiveOptionIndex; - if (!this.hasActiveOption()) { - // If this is the beginning of the user's keyboard navigation of the menu, then we'll focus - // either the first or last item. - nextActiveOptionIndex = amount < 0 ? this.matchingOptions.length - 1 : 0; - } else { - nextActiveOptionIndex = this.state.activeOptionIndex + amount; + if (!this.hasActiveOption()) { + // If this is the beginning of the user's keyboard navigation of the menu, then we'll focus + // either the first or last item. + nextActiveOptionIndex = amount < 0 ? matchingOptions.length - 1 : 0; + } else { + nextActiveOptionIndex = activeOptionIndex + amount; - if (nextActiveOptionIndex < 0) { - nextActiveOptionIndex = this.matchingOptions.length - 1; - } else if (nextActiveOptionIndex === this.matchingOptions.length) { - nextActiveOptionIndex = 0; + if (nextActiveOptionIndex < 0) { + nextActiveOptionIndex = matchingOptions.length - 1; + } else if (nextActiveOptionIndex === matchingOptions.length) { + nextActiveOptionIndex = 0; + } } - } - // Group titles are included in option list but are not selectable - // Skip group title options - const direction = amount > 0 ? 1 : -1; - while (this.matchingOptions[nextActiveOptionIndex].isGroupLabelOption) { - nextActiveOptionIndex = nextActiveOptionIndex + direction; + // Group titles are included in option list but are not selectable + // Skip group title options + const direction = amount > 0 ? 1 : -1; + while (matchingOptions[nextActiveOptionIndex].isGroupLabelOption) { + nextActiveOptionIndex = nextActiveOptionIndex + direction; - if (nextActiveOptionIndex < 0) { - nextActiveOptionIndex = this.matchingOptions.length - 1; - } else if (nextActiveOptionIndex === this.matchingOptions.length) { - nextActiveOptionIndex = 0; + if (nextActiveOptionIndex < 0) { + nextActiveOptionIndex = matchingOptions.length - 1; + } else if (nextActiveOptionIndex === matchingOptions.length) { + nextActiveOptionIndex = 0; + } } - } - this.setState({ - activeOptionIndex: nextActiveOptionIndex, + return { activeOptionIndex: nextActiveOptionIndex }; }); }, 200); @@ -294,10 +284,10 @@ export class EuiComboBox extends Component { doesSearchMatchOnlyOption = () => { const { searchValue } = this.state; - if (this.matchingOptions.length !== 1) { + if (this.state.matchingOptions.length !== 1) { return false; } - return this.matchingOptions[0].label.toLowerCase() === searchValue.toLowerCase(); + return this.state.matchingOptions[0].label.toLowerCase() === searchValue.toLowerCase(); }; areAllOptionsSelected = () => { @@ -491,35 +481,55 @@ export class EuiComboBox extends Component { }, 100); } - // TODO: React 16.3 - ideally refactor from class members into state - // and use getDerivedStateFromProps; otherwise componentDidUpdate - componentWillUpdate(nextProps, nextState) { + static getDerivedStateFromProps(nextProps, prevState) { const { options, selectedOptions } = nextProps; - const { searchValue } = nextState; - - if ( - options !== this.props.options - || selectedOptions !== this.props.selectedOptions - || searchValue !== this.props.searchValue - ) { - // Clear refs to options if the ones we can display changes. - this.options = []; - } + 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 = this.getMatchingOptions(options, selectedOptions, nextState.searchValue); - this.matchingOptions = matchingOptions; + const matchingOptions = getMatchingOptions(options, selectedOptions, searchValue, nextProps.async); + + return { matchingOptions }; + } + + updateMatchingOptionsIfDifferent(newMatchingOptions) { + const { matchingOptions } = this.state; + + let areOptionsDifferent = false; + + if (matchingOptions.length !== newMatchingOptions.length) { + areOptionsDifferent = true; + } else { + for (let i = 0; i < matchingOptions.length; i++) { + if (matchingOptions[i].label !== newMatchingOptions[i].label) { + areOptionsDifferent = true; + break; + } + } + } + + if (areOptionsDifferent) { + this.options = []; + this.setState({ matchingOptions: newMatchingOptions }); - if (!matchingOptions.length) { - // Prevent endless setState -> componentWillUpdate -> setState loop. - if (nextState.hasActiveOption) { - this.clearActiveOption(); + if (!newMatchingOptions.length) { + // Prevent endless setState -> componentWillUpdate -> setState loop. + if (this.state.hasActiveOption) { + this.clearActiveOption(); + } } } } componentDidUpdate() { + const { options, selectedOptions } = 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.focusActiveOption(); } @@ -573,7 +583,7 @@ export class EuiComboBox extends Component { selectedOptions={selectedOptions} onCreateOption={onCreateOption} searchValue={searchValue} - matchingOptions={this.matchingOptions} + matchingOptions={this.state.matchingOptions} listRef={this.optionsListRef} optionRef={this.optionRef} onOptionClick={this.onOptionClick} From a4914754618a0970bbabc63053142cdd57b88a21 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 30 May 2018 15:29:30 -0600 Subject: [PATCH 2/5] Refactored EuiComboBoxOptionsList and EuiComboBoxInput to React 16.3 lifecycle --- src/components/combo_box/combo_box_input/combo_box_input.js | 5 ++--- .../combo_box_options_list/combo_box_options_list.js | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/components/combo_box/combo_box_input/combo_box_input.js b/src/components/combo_box/combo_box_input/combo_box_input.js index edeb34cbcd8..feb0d52871f 100644 --- a/src/components/combo_box/combo_box_input/combo_box_input.js +++ b/src/components/combo_box/combo_box_input/combo_box_input.js @@ -61,9 +61,8 @@ export class EuiComboBoxInput extends Component { }); }; - // TODO: React 16.3 - componentDidUpdate - componentWillUpdate(nextProps) { - const { searchValue } = nextProps; + componentDidUpdate(prevProps) { + const { searchValue } = prevProps; // We need to update the position of everything if the user enters enough input to change // the size of the input. diff --git a/src/components/combo_box/combo_box_options_list/combo_box_options_list.js b/src/components/combo_box/combo_box_options_list/combo_box_options_list.js index 2ab4057f799..fc1bd11bd1b 100644 --- a/src/components/combo_box/combo_box_options_list/combo_box_options_list.js +++ b/src/components/combo_box/combo_box_options_list/combo_box_options_list.js @@ -65,9 +65,8 @@ export class EuiComboBoxOptionsList extends Component { window.addEventListener('resize', this.updatePosition); } - // TODO: React 16.3 - componentDidUpdate - componentWillUpdate(nextProps) { - const { options, selectedOptions, searchValue } = nextProps; + componentDidUpdate(prevProps) { + const { options, selectedOptions, searchValue } = prevProps; // We don't compare matchingOptions because that will result in a loop. if ( From ae2b1b6bb94d8481df10f51852bf27dee1148bf5 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 30 May 2018 16:30:15 -0600 Subject: [PATCH 3/5] Fix positioning of combobox in container --- src/components/combo_box/combo_box.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 4bc9d174c53..94bc1509a5c 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -102,6 +102,17 @@ export class EuiComboBox extends Component { const comboBoxBounds = this.comboBox.getBoundingClientRect(); + listBounds = { + bottom: listBounds.bottom, + height: listBounds.height, + left: comboBoxBounds.left, + right: comboBoxBounds.right, + top: listBounds.top, + width: comboBoxBounds.width, + x: listBounds.x, + y: listBounds.y, + }; + const { position, left, top } = calculatePopoverPosition(comboBoxBounds, listBounds, 'bottom', 0, ['bottom', 'top']); this.optionsList.style.top = `${top + window.scrollY}px`; @@ -111,16 +122,7 @@ export class EuiComboBox extends Component { // Cache for future calls. Assign values directly instead of destructuring because listBounds is // a DOMRect, not a JS object. this.setState({ - listBounds: { - bottom: listBounds.bottom, - height: listBounds.height, - left: comboBoxBounds.left, - right: comboBoxBounds.right, - top: listBounds.top, - width: comboBoxBounds.width, - x: listBounds.x, - y: listBounds.y, - }, + listBounds, width: comboBoxBounds.width, listPosition: position, }); From f5df841304bf4e180e3420118b1fda89a37e2ed2 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 31 May 2018 10:47:19 -0600 Subject: [PATCH 4/5] Fix bug where combo box always showed a clear button --- src/components/combo_box/combo_box_input/combo_box_input.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/combo_box/combo_box_input/combo_box_input.js b/src/components/combo_box/combo_box_input/combo_box_input.js index feb0d52871f..785d30724c9 100644 --- a/src/components/combo_box/combo_box_input/combo_box_input.js +++ b/src/components/combo_box/combo_box_input/combo_box_input.js @@ -149,9 +149,9 @@ export class EuiComboBoxInput extends Component { const clickProps = {}; - if (!isDisabled) { + if (!isDisabled && onClear && hasSelectedOptions) { clickProps.clear = { - onClick: hasSelectedOptions ? onClear : undefined, + onClick: onClear, }; } From 1487203181a8b0e1031ec1ed4747e44ec60aff44 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Fri, 1 Jun 2018 10:22:59 -0600 Subject: [PATCH 5/5] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d57fe18bb83..43755a9cfce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## [`master`](https://github.com/elastic/eui/tree/master) +**Bug fixes** - Removed `.nvmrc` file from published npm package ([#892](https://github.com/elastic/eui/pull/892)) +- `EuiComboBox` no longer shows the _clear_ icon when it's a no-op ([#890](https://github.com/elastic/eui/pull/890)) ## [`0.0.51`](https://github.com/elastic/eui/tree/v0.0.51)