From 424bd74f81a5a7d4ce4f4b96b82afb7136a21764 Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Tue, 3 Apr 2018 15:50:09 -0700 Subject: [PATCH 1/8] Fix bug where clicking a pill's close button would close the list, by refining the focus and blur logic. --- src/components/combo_box/combo_box.js | 41 +++++++++++++------ .../combo_box_input/combo_box_pill.js | 15 +++---- .../combo_box_option.js | 6 +-- 3 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 87b08d21667..034111dc4b8 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -190,6 +190,11 @@ export class EuiComboBox extends Component { } }; + focusSearchInput = () => { + this.clearActiveOption(); + this.searchInput.focus(); + }; + clearSearchValue = () => { this.onSearchChange(''); }; @@ -251,7 +256,19 @@ export class EuiComboBox extends Component { return flattenOptionGroups(options).length === selectedOptions.length; }; - onFocusChange = event => { + onFocus = () => { + document.addEventListener('click', this.onDocumentFocusChange); + document.addEventListener('focusin', this.onDocumentFocusChange); + this.openList(); + } + + onBlur = () => { + document.removeEventListener('click', this.onDocumentFocusChange); + document.removeEventListener('focusin', this.onDocumentFocusChange); + this.closeList(); + } + + onDocumentFocusChange = event => { // Close the list if the combo box has lost focus. if ( this.comboBox === event.target @@ -264,7 +281,11 @@ export class EuiComboBox extends Component { // Wait for the DOM to update. requestAnimationFrame(() => { - this.closeList(); + if (document.activeElement === this.searchInput) { + return; + } + + this.onBlur(); }); }; @@ -287,8 +308,7 @@ export class EuiComboBox extends Component { case ESCAPE: // Move focus from options list to input. if (this.hasActiveOption()) { - this.clearActiveOption(); - this.searchInput.focus(); + this.focusSearchInput(); } break; @@ -319,14 +339,14 @@ export class EuiComboBox extends Component { onAddOption = (addedOption) => { const { onChange, selectedOptions, singleSelection } = this.props; onChange(singleSelection ? [addedOption] : selectedOptions.concat(addedOption)); - this.clearActiveOption(); this.clearSearchValue(); - this.searchInput.focus(); + this.focusSearchInput(); }; onRemoveOption = (removedOption) => { const { onChange, selectedOptions } = this.props; onChange(selectedOptions.filter(option => option !== removedOption)); + this.focusSearchInput(); }; onComboBoxClick = () => { @@ -381,9 +401,6 @@ export class EuiComboBox extends Component { }; componentDidMount() { - document.addEventListener('click', this.onFocusChange); - document.addEventListener('focusin', this.onFocusChange); - // TODO: This will need to be called once the actual stylesheet loads. setTimeout(() => { this.autoSizeInput.copyInputStyles(); @@ -419,8 +436,8 @@ export class EuiComboBox extends Component { } componentWillUnmount() { - document.removeEventListener('click', this.onFocusChange); - document.removeEventListener('focusin', this.onFocusChange); + document.removeEventListener('click', this.onDocumentFocusChange); + document.removeEventListener('focusin', this.onDocumentFocusChange); } render() { @@ -491,7 +508,7 @@ export class EuiComboBox extends Component { onRemoveOption={this.onRemoveOption} onClick={this.onComboBoxClick} onChange={this.onSearchChange} - onFocus={this.openList} + onFocus={this.onFocus} value={value} searchValue={searchValue} autoSizeInputRef={this.autoSizeInputRef} diff --git a/src/components/combo_box/combo_box_input/combo_box_pill.js b/src/components/combo_box/combo_box_input/combo_box_pill.js index 697ed24c281..bb637b668e7 100644 --- a/src/components/combo_box/combo_box_input/combo_box_pill.js +++ b/src/components/combo_box/combo_box_input/combo_box_pill.js @@ -21,19 +21,16 @@ export class EuiComboBoxPill extends Component { color: 'hollow', }; - onCloseButtonClick(option, e) { - // Prevent the combo box from losing focus. - e.preventDefault(); - e.stopPropagation(); - e.nativeEvent.stopImmediatePropagation(); - this.props.onClose(option) - } + onCloseButtonClick = () => { + const { onClose, option } = this.props; + onClose(option); + }; render() { const { children, className, - option, + option, // eslint-disable-line no-unused-vars onClose, // eslint-disable-line no-unused-vars color, ...rest @@ -44,7 +41,7 @@ export class EuiComboBoxPill extends Component { { - // Prevent the combo box from losing focus. - e.preventDefault(); - e.stopPropagation(); - e.nativeEvent.stopImmediatePropagation(); + onClick = () => { const { onClick, option } = this.props; onClick(option); }; From 1bdea74f0ebb9275cf964c87aeb7c45822784698 Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Tue, 3 Apr 2018 16:13:26 -0700 Subject: [PATCH 2/8] Fix bug with positioning the list when there's no scrollbar, caused by the absolute position in the CSS. --- src/components/combo_box/combo_box.js | 2 +- .../combo_box_options_list/_combo_box_options_list.scss | 8 ++++++-- .../combo_box_options_list/combo_box_options_list.js | 3 --- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 034111dc4b8..2c86663765a 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -116,7 +116,7 @@ export class EuiComboBox extends Component { const { position, left, top } = calculatePopoverPosition(comboBoxBounds, this.listBounds, 'bottom', 0, ['bottom', 'top']); - this.optionsList.style.top = `${top + window.scrollY}px`; + this.optionsList.style.top = `${top}px`; this.optionsList.style.left = `${left}px`; this.optionsList.style.width = `${comboBoxBounds.width}px`; diff --git a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss index 6bc0c22485a..b5ebe98b712 100644 --- a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss +++ b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss @@ -1,11 +1,15 @@ /** - * 1. Make width match that of the input. + * 1. Make width match that of the input and tweak position to match. + * 2. We can't use absolute position here because this will cause a scrollbar to show up when + * the portal is appended to the body. This throws off our logic for positioning the + * list beneath the input. */ .euiComboBoxOptionsList { @include euiFormControlSize; box-sizing: content-box; /* 1 */ + margin-left: -1px; /* 1 */ z-index: $euiZComboBox; - position: absolute; + position: fixed; /* 2 */ } .euiComboBoxOptionsList--bottom { 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 9f9e6d62b3d..651ddd89da6 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 @@ -46,8 +46,6 @@ export class EuiComboBoxOptionsList extends Component { }; componentDidMount() { - document.body.classList.add('euiBody-hasPortalContent'); - this.updatePosition(); window.addEventListener('resize', this.updatePosition); window.addEventListener('scroll', this.updatePosition); @@ -67,7 +65,6 @@ export class EuiComboBoxOptionsList extends Component { } componentWillUnmount() { - document.body.classList.remove('euiBody-hasPortalContent'); window.removeEventListener('resize', this.updatePosition); window.removeEventListener('scroll', this.updatePosition); } From 5d712552cc82d72f5d63e565b8c7f67c1885639d Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Tue, 3 Apr 2018 16:36:25 -0700 Subject: [PATCH 3/8] Add support for validation to EuiComboBox. --- .../src/views/combo_box/combo_box_example.js | 2 +- .../views/combo_box/custom_options_only.js | 53 ++++++++++++++----- src/components/combo_box/_combo_box.scss | 6 +++ src/components/combo_box/combo_box.js | 11 +++- .../combo_box_input/combo_box_input.js | 28 +++++----- 5 files changed, 71 insertions(+), 29 deletions(-) diff --git a/src-docs/src/views/combo_box/combo_box_example.js b/src-docs/src/views/combo_box/combo_box_example.js index e35a35c5ec0..f13e55ba154 100644 --- a/src-docs/src/views/combo_box/combo_box_example.js +++ b/src-docs/src/views/combo_box/combo_box_example.js @@ -198,7 +198,7 @@ export const ComboBoxExample = { props: { EuiComboBox }, demo: , }, { - title: 'Hiding suggestions', + title: 'Custom options only, with validation', source: [{ type: GuideSectionTypes.JS, code: customOptionsOnlySource, diff --git a/src-docs/src/views/combo_box/custom_options_only.js b/src-docs/src/views/combo_box/custom_options_only.js index a2d6bed25c0..dd150bb6086 100644 --- a/src-docs/src/views/combo_box/custom_options_only.js +++ b/src-docs/src/views/combo_box/custom_options_only.js @@ -2,22 +2,28 @@ import React, { Component } from 'react'; import { EuiComboBox, + EuiFormRow, } from '../../../../src/components'; +const isValid = (value) => { + // Only allow letters. No spaces, numbers, or special characters. + return value.match(/^[a-zA-Z]+$/) !== null; +}; + export default class extends Component { constructor(props) { super(props); this.state = { + isInvalid: false, selectedOptions: [], }; } onCreateOption = (searchValue) => { - const normalizedSearchValue = searchValue.trim().toLowerCase(); - - if (!normalizedSearchValue) { - return; + if (!isValid(searchValue)) { + // Return false to explicitly reject the user's input. + return false; } const newOption = { @@ -30,22 +36,45 @@ export default class extends Component { })); }; + onSearchChange = (searchValue) => { + if (!searchValue) { + this.setState({ + isInvalid: false, + }); + + return; + } + + this.setState({ + isInvalid: !isValid(searchValue), + }); + }; + onChange = (selectedOptions) => { this.setState({ selectedOptions, + isInvalid: false, }); }; render() { - const { selectedOptions } = this.state; + const { selectedOptions, isInvalid } = this.state; return ( - + + + ); } } diff --git a/src/components/combo_box/_combo_box.scss b/src/components/combo_box/_combo_box.scss index 6447beb2f3b..e42df2a9601 100644 --- a/src/components/combo_box/_combo_box.scss +++ b/src/components/combo_box/_combo_box.scss @@ -54,4 +54,10 @@ inset 0 -2px 0 0 $euiColorPrimary; } } + + &.euiComboBox-isInvalid { + .euiComboBox__inputWrap { + @include euiFormControlInvalidStyle; + } + } } diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 2c86663765a..7110d905bec 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -37,6 +37,7 @@ export class EuiComboBox extends Component { onSearchChange: PropTypes.func, onCreateOption: PropTypes.func, renderOption: PropTypes.func, + isInvalid: PropTypes.bool, } static defaultProps = { @@ -234,7 +235,13 @@ export class EuiComboBox extends Component { // Add new custom pill if this is custom input, even if it partially matches an option.. if (!this.hasActiveOption() || this.doesSearchMatchOnlyOption()) { - this.props.onCreateOption(this.state.searchValue, flattenOptionGroups(this.props.options)); + const isOptionCreated = this.props.onCreateOption(this.state.searchValue, flattenOptionGroups(this.props.options)); + + // Expect the consumer to be explicit in rejecting a custom option. + if (isOptionCreated === false) { + return; + } + this.clearSearchValue(); } }; @@ -455,6 +462,7 @@ export class EuiComboBox extends Component { onChange, // eslint-disable-line no-unused-vars onSearchChange, // eslint-disable-line no-unused-vars async, // eslint-disable-line no-unused-vars + isInvalid, ...rest } = this.props; @@ -462,6 +470,7 @@ export class EuiComboBox extends Component { const classes = classNames('euiComboBox', className, { 'euiComboBox-isOpen': isListOpen, + 'euiComboBox-isInvalid': isInvalid, }); const value = selectedOptions.map(selectedOption => selectedOption.label).join(', '); 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 0944c876668..244d08dd4c3 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 @@ -3,7 +3,7 @@ import PropTypes from 'prop-types'; import AutosizeInput from 'react-input-autosize'; import { EuiScreenReaderOnly } from '../../accessibility'; -import { EuiFormControlLayout, EuiValidatableControl } from '../../form'; +import { EuiFormControlLayout } from '../../form'; import { EuiComboBoxPill } from './combo_box_pill'; import { htmlIdGenerator } from '../../../services'; @@ -143,20 +143,18 @@ export class EuiComboBoxInput extends Component { > {pills} {placeholderMessage} - - onChange(e.target.value)} - value={searchValue} - ref={autoSizeInputRef} - inputRef={inputRef} - /> - + onChange(e.target.value)} + value={searchValue} + ref={autoSizeInputRef} + inputRef={inputRef} + /> {removeOptionMessage} From 3e5b3635e49ad8ccf1f148868e718fb8ffc657ac Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Tue, 3 Apr 2018 16:42:35 -0700 Subject: [PATCH 4/8] Update CHANGELOG. --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70c96f19a85..bdc883d4602 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ - Modified drop shadow intensities and color. ([#607](https://github.com/elastic/eui/pull/607)) - Removed extraneous `global_styling/mixins/_forms.scss` file and importing the correct files in the `filter_group.scss` and `combo_box.scss` files. ([#609](https://github.com/elastic/eui/pull/609)) +- Added `isInvalid` prop to `EuiComboBox` ([#631](https://github.com/elastic/eui/pull/631)) +- Added support for rejecting user input by returning `false` from the `onCreateOption` prop of `EuiComboBox` ([#631](https://github.com/elastic/eui/pull/631)) **Bug fixes** @@ -9,6 +11,8 @@ - `EuiSelect` can pass any node as a value rather than just a string ([#603](https://github.com/elastic/eui/pull/603)) - Fixed a typo in the flex TypeScript definition ([#629](https://github.com/elastic/eui/pull/629)) - Fixed `EuiComboBox` bug in which the options list wouldn't always match the width of the input ([#611](https://github.com/elastic/eui/pull/611)) +- Fixed `EuiComboBox` bug in which opening the combo box when there's no scrollbar on the window would result in the list being positioned incorrectly ([#631](https://github.com/elastic/eui/pull/631)) +- Fixed `EuiComboBox` bug in which clicking a pill's close button would close the list ([#631](https://github.com/elastic/eui/pull/631)) # [`0.0.37`](https://github.com/elastic/eui/tree/v0.0.37) From 9ad1c8278afa6480f518e84ec5bef527b2122d70 Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Tue, 3 Apr 2018 17:25:35 -0700 Subject: [PATCH 5/8] Switch back to absolute positioning and remove scroll listener to improve performance, particularly on IE11. --- src/components/combo_box/combo_box.js | 2 +- .../combo_box_options_list/_combo_box_options_list.scss | 7 ++++--- .../combo_box_options_list/combo_box_options_list.js | 2 -- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 7110d905bec..49851787e4f 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -117,7 +117,7 @@ export class EuiComboBox extends Component { const { position, left, top } = calculatePopoverPosition(comboBoxBounds, this.listBounds, 'bottom', 0, ['bottom', 'top']); - this.optionsList.style.top = `${top}px`; + this.optionsList.style.top = `${top + window.scrollY}px`; this.optionsList.style.left = `${left}px`; this.optionsList.style.width = `${comboBoxBounds.width}px`; diff --git a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss index b5ebe98b712..2f3fce45414 100644 --- a/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss +++ b/src/components/combo_box/combo_box_options_list/_combo_box_options_list.scss @@ -1,7 +1,7 @@ /** * 1. Make width match that of the input and tweak position to match. - * 2. We can't use absolute position here because this will cause a scrollbar to show up when - * the portal is appended to the body. This throws off our logic for positioning the + * 2. Put the list at the top of the screen, otherwise it will cause a scrollbar to show up when + * the portal is appended to the body. This would throw off our logic for positioning the * list beneath the input. */ .euiComboBoxOptionsList { @@ -9,7 +9,8 @@ box-sizing: content-box; /* 1 */ margin-left: -1px; /* 1 */ z-index: $euiZComboBox; - position: fixed; /* 2 */ + position: absolute; /* 2 */ + top: 0; /* 2 */ } .euiComboBoxOptionsList--bottom { 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 651ddd89da6..de1bbeb4820 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 @@ -48,7 +48,6 @@ export class EuiComboBoxOptionsList extends Component { componentDidMount() { this.updatePosition(); window.addEventListener('resize', this.updatePosition); - window.addEventListener('scroll', this.updatePosition); } componentWillUpdate(nextProps) { @@ -66,7 +65,6 @@ export class EuiComboBoxOptionsList extends Component { componentWillUnmount() { window.removeEventListener('resize', this.updatePosition); - window.removeEventListener('scroll', this.updatePosition); } listRef = node => { From c8a311637df841adf80badd8eaa9dbbef8261e55 Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Tue, 3 Apr 2018 19:56:29 -0700 Subject: [PATCH 6/8] Use !important to override CSS top property. --- src/components/combo_box/combo_box.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 49851787e4f..982799b387b 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -117,9 +117,12 @@ export class EuiComboBox extends Component { const { position, left, top } = calculatePopoverPosition(comboBoxBounds, this.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`; + // We use !important to override the style set by the CSS class. We have to use setAttribute + // over setting properties on the style object because that won't work with !important. + this.optionsList.setAttribute( + 'style', + `top: ${top + window.scrollY}px !important; left: ${left}px; width: ${comboBoxBounds.width}px;` + ); this.setState({ listPosition: position, From 9d82e4ca8d12bfd1dd1d355edea9fb05c2711319 Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Wed, 4 Apr 2018 11:04:14 -0700 Subject: [PATCH 7/8] Revert "Use !important to override CSS top property." This reverts commit c8a311637df841adf80badd8eaa9dbbef8261e55. --- src/components/combo_box/combo_box.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/components/combo_box/combo_box.js b/src/components/combo_box/combo_box.js index 982799b387b..49851787e4f 100644 --- a/src/components/combo_box/combo_box.js +++ b/src/components/combo_box/combo_box.js @@ -117,12 +117,9 @@ export class EuiComboBox extends Component { const { position, left, top } = calculatePopoverPosition(comboBoxBounds, this.listBounds, 'bottom', 0, ['bottom', 'top']); - // We use !important to override the style set by the CSS class. We have to use setAttribute - // over setting properties on the style object because that won't work with !important. - this.optionsList.setAttribute( - 'style', - `top: ${top + window.scrollY}px !important; left: ${left}px; width: ${comboBoxBounds.width}px;` - ); + this.optionsList.style.top = `${top + window.scrollY}px`; + this.optionsList.style.left = `${left}px`; + this.optionsList.style.width = `${comboBoxBounds.width}px`; this.setState({ listPosition: position, From b5c57d2ab8e5830630492915338eeb85a2fde8b1 Mon Sep 17 00:00:00 2001 From: CJ Cenizal Date: Wed, 4 Apr 2018 11:18:26 -0700 Subject: [PATCH 8/8] Fix bug with logic managing the euiBody-hasPortalContent class. --- CHANGELOG.md | 1 + .../combo_box_options_list/combo_box_options_list.js | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdc883d4602..10033a106a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Fixed `EuiComboBox` bug in which the options list wouldn't always match the width of the input ([#611](https://github.com/elastic/eui/pull/611)) - Fixed `EuiComboBox` bug in which opening the combo box when there's no scrollbar on the window would result in the list being positioned incorrectly ([#631](https://github.com/elastic/eui/pull/631)) - Fixed `EuiComboBox` bug in which clicking a pill's close button would close the list ([#631](https://github.com/elastic/eui/pull/631)) +- Fixed `EuiComboBox` bug in which moving focus from one combo box to another would remove the `euiBody-hasPortalContent` class from the body. ([#631](https://github.com/elastic/eui/pull/631)) # [`0.0.37`](https://github.com/elastic/eui/tree/v0.0.37) 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 de1bbeb4820..e23be602202 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 @@ -46,6 +46,11 @@ export class EuiComboBoxOptionsList extends Component { }; componentDidMount() { + // Wait a frame, otherwise moving focus from one combo box to another will result in the class + // being removed from the body. + requestAnimationFrame(() => { + document.body.classList.add('euiBody-hasPortalContent'); + }); this.updatePosition(); window.addEventListener('resize', this.updatePosition); } @@ -64,6 +69,7 @@ export class EuiComboBoxOptionsList extends Component { } componentWillUnmount() { + document.body.classList.remove('euiBody-hasPortalContent'); window.removeEventListener('resize', this.updatePosition); }