From cccb17274e2c6a89e72a676ff5034fb4d4d35598 Mon Sep 17 00:00:00 2001 From: Timothy Galfin <79439202+tgalfin@users.noreply.github.com> Date: Mon, 12 Aug 2024 19:13:16 -0700 Subject: [PATCH 01/10] feat: add option to disable field value selection auto sort --- .../src/views/search_bar/props_info.js | 6 ++ .../field_value_selection_filter.spec.tsx | 81 +++++++++++++++++++ .../filters/field_value_selection_filter.tsx | 14 +++- 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/packages/eui/src-docs/src/views/search_bar/props_info.js b/packages/eui/src-docs/src/views/search_bar/props_info.js index 6616f9270f0..fc01d8ee1dd 100644 --- a/packages/eui/src-docs/src/views/search_bar/props_info.js +++ b/packages/eui/src-docs/src/views/search_bar/props_info.js @@ -291,6 +291,12 @@ export const propsInfo = { defaultValue: { value: 'eq' }, type: { name: 'eq | exact | gt | gte | lt | lte' }, }, + autoSortOptions: { + description: 'Should the dropdown auto sort selections to the top.', + required: false, + defaultValue: { value: 'true' }, + type: { name: 'boolean' }, + }, }, }, }, diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx index 0189dd6e18c..fd07c1db1a5 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx @@ -453,4 +453,85 @@ describe('FieldValueSelectionFilter', () => { .eq(0) .should('have.attr', 'title', 'Bug'); }); + + it('sorts selected options to the top by default', () => { + const props: FieldValueSelectionFilterProps = { + ...requiredProps, + index: 0, + onChange: () => {}, + query: Query.parse('tag_3:bug'), + config: { + type: 'field_value_selection', + name: 'Tag', + options: [ + { + field: 'tag', + value: 'feature', + }, + { + field: 'tag_2', + value: 'test', + name: 'Text', + }, + { + field: 'tag_3', + value: 'bug', + name: 'Bug', + view:
bug
, + }, + ], + }, + }; + + cy.mount(); + + cy.get('button').click(); + cy.get('.euiNotificationBadge').should('not.be.undefined'); + cy.get('[data-test-subj="euiSelectableList"] li') + .eq(0) + .should('have.attr', 'title', 'Bug'); + }); + + it('does not sort selected options to the top when disabled via config', () => { + const props: FieldValueSelectionFilterProps = { + ...requiredProps, + index: 0, + onChange: () => {}, + query: Query.parse('tag_3:bug'), + config: { + type: 'field_value_selection', + name: 'Tag', + options: [ + { + field: 'tag', + value: 'feature', + name: 'Feature', + }, + { + field: 'tag_2', + value: 'test', + name: 'Text', + }, + { + field: 'tag_3', + value: 'bug', + name: 'Bug', + view:
bug
, + }, + ], + autoSortOptions: false, + }, + }; + + cy.mount(); + + cy.get('button').click(); + cy.get('.euiNotificationBadge').should('not.be.undefined'); + cy.get('[data-test-subj="euiSelectableList"] li') + .eq(0) + .should('have.attr', 'title', 'Feature'); + cy.get('[data-test-subj="euiSelectableList"] li') + .eq(2) + .should('have.attr', 'title', 'Bug'); + }); }); diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index 9ceef0f9a6b..3915c2599c0 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -53,6 +53,7 @@ export interface FieldValueSelectionFilterConfigType { available?: () => boolean; autoClose?: boolean; operator?: OperatorType; + autoSortOptions?: boolean; } export interface FieldValueSelectionFilterProps { @@ -67,6 +68,7 @@ const defaults = { multiSelect: true, filterWith: 'prefix', searchThreshold: 10, + autoSortOptions: true, }, }; @@ -132,10 +134,12 @@ export class FieldValueSelectionFilter extends Component< on: FieldValueOptionType[]; off: FieldValueOptionType[]; rest: FieldValueOptionType[]; + all: FieldValueOptionType[]; } = { on: [], off: [], rest: [], + all: [], }; const { query, config } = this.props; @@ -158,17 +162,25 @@ export class FieldValueSelectionFilter extends Component< } else { items.off.push(op); } + items.all.push(op); } return; }); } + const autoSortOptions = + this.props.config.autoSortOptions === undefined + ? defaults.config.autoSortOptions + : this.props.config.autoSortOptions; + this.setState({ error: null, activeItems: items.on, options: { all: options, - shown: [...items.on, ...items.off, ...items.rest], + shown: autoSortOptions + ? [...items.on, ...items.off, ...items.rest] + : [...items.all], }, }); }) From 06eca3730673b3213cb4412084914ceb050afd23 Mon Sep 17 00:00:00 2001 From: Timothy Galfin <79439202+tgalfin@users.noreply.github.com> Date: Tue, 13 Aug 2024 09:15:32 -0700 Subject: [PATCH 02/10] add changelog --- packages/eui/changelogs/upcoming/7958.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 packages/eui/changelogs/upcoming/7958.md diff --git a/packages/eui/changelogs/upcoming/7958.md b/packages/eui/changelogs/upcoming/7958.md new file mode 100644 index 00000000000..0d3075f11b6 --- /dev/null +++ b/packages/eui/changelogs/upcoming/7958.md @@ -0,0 +1,2 @@ +- Added/Updated ... +- Added a new optional prop, `autoSortOptions`, to `FieldValueSelectionFilter` to allow users to turn specify whether or not to auto sort selected items to the top ([#7958](https://github.com/elastic/eui/pull/7958)) From e24b3c52342252c58247ecf85d14508abb6a4387 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 28 Aug 2024 20:04:27 -0700 Subject: [PATCH 03/10] [PR feedback] Minor test cleanup - move to a `describe` block with config key + move closer to `autoClose` - remove unnecessary `...requiredProps`, reuse `staticOptions` const - clean up assertions slightly --- .../field_value_selection_filter.spec.tsx | 130 +++++++----------- 1 file changed, 49 insertions(+), 81 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx index fd07c1db1a5..962322d4e12 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx @@ -338,6 +338,55 @@ describe('FieldValueSelectionFilter', () => { }); }); + describe('autoSortOptions', () => { + it('sorts selected options to the top by default', () => { + const props: FieldValueSelectionFilterProps = { + index: 0, + onChange: () => {}, + query: Query.parse('tag:bug'), + config: { + type: 'field_value_selection', + field: 'tag', + name: 'Tag', + options: staticOptions, + }, + }; + cy.mount(); + cy.get('.euiNotificationBadge').should('exist'); + + cy.get('button').click(); + const getOptions = () => cy.get('.euiSelectableListItem'); + + getOptions().should('have.length', 3); + getOptions().eq(0).should('have.attr', 'title', 'Bug'); + getOptions().eq(1).should('have.attr', 'title', 'feature'); + }); + + it('does not sort selected options to the top when set to false', () => { + const props: FieldValueSelectionFilterProps = { + index: 0, + onChange: () => {}, + query: Query.parse('tag:bug'), + config: { + type: 'field_value_selection', + field: 'tag', + name: 'Tag', + options: staticOptions, + autoSortOptions: false, + }, + }; + cy.mount(); + cy.get('.euiNotificationBadge').should('exist'); + + cy.get('button').click(); + const getOptions = () => cy.get('.euiSelectableListItem'); + + getOptions().should('have.length', 3); + getOptions().eq(2).should('have.attr', 'title', 'Bug'); + getOptions().eq(0).should('have.attr', 'title', 'feature'); + }); + }); + it('has inactive filters, field is global', () => { const props: FieldValueSelectionFilterProps = { ...requiredProps, @@ -453,85 +502,4 @@ describe('FieldValueSelectionFilter', () => { .eq(0) .should('have.attr', 'title', 'Bug'); }); - - it('sorts selected options to the top by default', () => { - const props: FieldValueSelectionFilterProps = { - ...requiredProps, - index: 0, - onChange: () => {}, - query: Query.parse('tag_3:bug'), - config: { - type: 'field_value_selection', - name: 'Tag', - options: [ - { - field: 'tag', - value: 'feature', - }, - { - field: 'tag_2', - value: 'test', - name: 'Text', - }, - { - field: 'tag_3', - value: 'bug', - name: 'Bug', - view:
bug
, - }, - ], - }, - }; - - cy.mount(); - - cy.get('button').click(); - cy.get('.euiNotificationBadge').should('not.be.undefined'); - cy.get('[data-test-subj="euiSelectableList"] li') - .eq(0) - .should('have.attr', 'title', 'Bug'); - }); - - it('does not sort selected options to the top when disabled via config', () => { - const props: FieldValueSelectionFilterProps = { - ...requiredProps, - index: 0, - onChange: () => {}, - query: Query.parse('tag_3:bug'), - config: { - type: 'field_value_selection', - name: 'Tag', - options: [ - { - field: 'tag', - value: 'feature', - name: 'Feature', - }, - { - field: 'tag_2', - value: 'test', - name: 'Text', - }, - { - field: 'tag_3', - value: 'bug', - name: 'Bug', - view:
bug
, - }, - ], - autoSortOptions: false, - }, - }; - - cy.mount(); - - cy.get('button').click(); - cy.get('.euiNotificationBadge').should('not.be.undefined'); - cy.get('[data-test-subj="euiSelectableList"] li') - .eq(0) - .should('have.attr', 'title', 'Feature'); - cy.get('[data-test-subj="euiSelectableList"] li') - .eq(2) - .should('have.attr', 'title', 'Bug'); - }); }); From 74e712e498372c2d2388876d03b2ad0732cf230a Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 28 Aug 2024 20:07:04 -0700 Subject: [PATCH 04/10] [PR feedback] Copy/docs pass - Tweak props table order - Minor wordsmithing --- packages/eui/changelogs/upcoming/7958.md | 3 +- .../src/views/search_bar/props_info.js | 47 ++++++++++--------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/eui/changelogs/upcoming/7958.md b/packages/eui/changelogs/upcoming/7958.md index 0d3075f11b6..92b4aaf4f32 100644 --- a/packages/eui/changelogs/upcoming/7958.md +++ b/packages/eui/changelogs/upcoming/7958.md @@ -1,2 +1 @@ -- Added/Updated ... -- Added a new optional prop, `autoSortOptions`, to `FieldValueSelectionFilter` to allow users to turn specify whether or not to auto sort selected items to the top ([#7958](https://github.com/elastic/eui/pull/7958)) +- Updated `EuiSearchBar`'s `field_value_selection` filter type with a new `autoSortOptions` config, allowing consumers to configure whether or not selected options are automatically sorted to the top of the filter list diff --git a/packages/eui/src-docs/src/views/search_bar/props_info.js b/packages/eui/src-docs/src/views/search_bar/props_info.js index fc01d8ee1dd..8172e69d545 100644 --- a/packages/eui/src-docs/src/views/search_bar/props_info.js +++ b/packages/eui/src-docs/src/views/search_bar/props_info.js @@ -224,6 +224,12 @@ export const propsInfo = { required: true, type: { name: '#FieldValueOption[] | () => #FieldValueOption[]' }, }, + available: { + description: + 'A callback that defines whether this filter is currently available', + required: false, + type: { name: '() => boolean' }, + }, filterWith: { description: 'Specify how user input in the option dropdown will filter the available options.', @@ -249,19 +255,12 @@ export const propsInfo = { defaultValue: { value: 'true ("and")' }, type: { name: 'boolean | "or" | "and"' }, }, - loadingMessage: { - description: - 'The message that will be shown while loading the options', - required: false, - defaultValue: { value: 'Loading...' }, - type: { name: 'string' }, - }, - noOptionsMessage: { + operator: { description: - 'The message that will be shown when no options are found', + 'What operator should be used when adding selection to the search bar.', required: false, - defaultValue: { value: 'No options found' }, - type: { name: 'string' }, + defaultValue: { value: 'eq' }, + type: { name: 'eq | exact | gt | gte | lt | lte' }, }, searchThreshold: { description: @@ -271,28 +270,30 @@ export const propsInfo = { defaultValue: { value: '10' }, type: { name: 'number' }, }, - available: { + noOptionsMessage: { description: - 'A callback that defines whether this filter is currently available', + 'The message that will be shown when no options are found', required: false, - type: { name: '() => boolean' }, + defaultValue: { value: 'No options found' }, + type: { name: 'string' }, }, - autoClose: { + loadingMessage: { description: - 'Should the dropdown close after the user selects a value. If not explicitly passed, will auto-close for single selection and remain open for multi-selection.', + 'The message that will be shown while loading the options', required: false, - defaultValue: { value: 'true' }, - type: { name: 'boolean' }, + defaultValue: { value: 'Loading...' }, + type: { name: 'string' }, }, - operator: { + autoClose: { description: - 'What operator should be used when adding selection to the search bar.', + 'Whether the dropdown should close after the user selects a value. If not explicitly passed, will auto-close for single selection and remain open for multi-selection.', required: false, - defaultValue: { value: 'eq' }, - type: { name: 'eq | exact | gt | gte | lt | lte' }, + defaultValue: { value: 'true' }, + type: { name: 'boolean' }, }, autoSortOptions: { - description: 'Should the dropdown auto sort selections to the top.', + description: + 'Whether selected options (on and off) should be shown at the top of the filters list', required: false, defaultValue: { value: 'true' }, type: { name: 'boolean' }, From 11e4b0e142e3e13a125ce029bddbf5c31c4e416e Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 28 Aug 2024 20:07:22 -0700 Subject: [PATCH 05/10] Docs/Storybook examples + fix filters stories file name --- packages/eui/src-docs/src/views/search_bar/search_bar_filters.js | 1 + packages/eui/src/components/search_bar/search_bar.stories.tsx | 1 + ...search_bar_filters.stories.tsx => search_filters.stories.tsx} | 1 + 3 files changed, 3 insertions(+) rename packages/eui/src/components/search_bar/{search_bar_filters.stories.tsx => search_filters.stories.tsx} (98%) diff --git a/packages/eui/src-docs/src/views/search_bar/search_bar_filters.js b/packages/eui/src-docs/src/views/search_bar/search_bar_filters.js index a615d5c96bf..bbcb3814a1f 100644 --- a/packages/eui/src-docs/src/views/search_bar/search_bar_filters.js +++ b/packages/eui/src-docs/src/views/search_bar/search_bar_filters.js @@ -204,6 +204,7 @@ export const SearchBarFilters = () => { value: tag.name, view: {tag.name}, })), + autoSortOptions: false, }, { type: 'custom_component', diff --git a/packages/eui/src/components/search_bar/search_bar.stories.tsx b/packages/eui/src/components/search_bar/search_bar.stories.tsx index cdce9bc461e..f30f79ce91c 100644 --- a/packages/eui/src/components/search_bar/search_bar.stories.tsx +++ b/packages/eui/src/components/search_bar/search_bar.stories.tsx @@ -148,6 +148,7 @@ export const Playground: Story = { ); }, 2000); }), + autoSortOptions: true, }, ], // casting to any to allow for easier teasting/QA via toggle switch diff --git a/packages/eui/src/components/search_bar/search_bar_filters.stories.tsx b/packages/eui/src/components/search_bar/search_filters.stories.tsx similarity index 98% rename from packages/eui/src/components/search_bar/search_bar_filters.stories.tsx rename to packages/eui/src/components/search_bar/search_filters.stories.tsx index e4897b9e223..3c54f26ddff 100644 --- a/packages/eui/src/components/search_bar/search_bar_filters.stories.tsx +++ b/packages/eui/src/components/search_bar/search_filters.stories.tsx @@ -88,6 +88,7 @@ export const Playground: Story = { ); }, 2000); }), + autoSortOptions: true, }, ], // setting up props for easier testing/QA From afe70fe0ea3323ace6769ff626335eed66222f70 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 29 Aug 2024 10:27:00 -0700 Subject: [PATCH 06/10] Simplify/clean up internal options state/naming - instead of `all/shown`, use `unsorted` and `sorted` to reflect new prop usage --- .../filters/field_value_selection_filter.tsx | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index 3915c2599c0..00b9907b230 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -76,8 +76,8 @@ interface State { popoverOpen: boolean; error: string | null; options: { - all: FieldValueOptionType[]; - shown: FieldValueOptionType[]; + unsorted: FieldValueOptionType[]; + sorted: FieldValueOptionType[]; } | null; cachedOptions?: FieldValueOptionType[] | null; activeItems: FieldValueOptionType[]; @@ -93,8 +93,8 @@ export class FieldValueSelectionFilter extends Component< const preloadedOptions = isArray(options) ? { - all: options, - shown: options, + unsorted: options, + sorted: options, } : null; this.state = { @@ -134,12 +134,10 @@ export class FieldValueSelectionFilter extends Component< on: FieldValueOptionType[]; off: FieldValueOptionType[]; rest: FieldValueOptionType[]; - all: FieldValueOptionType[]; } = { on: [], off: [], rest: [], - all: [], }; const { query, config } = this.props; @@ -162,25 +160,17 @@ export class FieldValueSelectionFilter extends Component< } else { items.off.push(op); } - items.all.push(op); } return; }); } - const autoSortOptions = - this.props.config.autoSortOptions === undefined - ? defaults.config.autoSortOptions - : this.props.config.autoSortOptions; - this.setState({ error: null, activeItems: items.on, options: { - all: options, - shown: autoSortOptions - ? [...items.on, ...items.off, ...items.rest] - : [...items.all], + unsorted: options, + sorted: [...items.on, ...items.off, ...items.rest], }, }); }) @@ -317,9 +307,15 @@ export class FieldValueSelectionFilter extends Component< const { query, config } = this.props; const multiSelect = this.resolveMultiSelect(); + const autoSortOptions = + config.autoSortOptions ?? defaults.config.autoSortOptions; + const options = autoSortOptions + ? this.state.options?.sorted + : this.state.options?.unsorted; + const activeTop = this.isActiveField(config.field); - const activeItem = this.state.options - ? this.state.options.all.some((item) => this.isActiveField(item.field)) + const activeItem = options + ? options.some((item) => this.isActiveField(item.field)) : false; const activeItemsCount = this.state.activeItems.length; @@ -338,8 +334,8 @@ export class FieldValueSelectionFilter extends Component< ); - const items = this.state.options - ? this.state.options.shown.map((option) => { + const items = options + ? options.map((option) => { const optionField = option.field || config.field; if (optionField == null) { @@ -369,8 +365,7 @@ export class FieldValueSelectionFilter extends Component< : []; const threshold = config.searchThreshold || defaults.config.searchThreshold; - const isOverSearchThreshold = - this.state.options && this.state.options.all.length >= threshold; + const isOverSearchThreshold = options && options.length >= threshold; let searchProps: ExclusiveUnion< { searchable: false }, @@ -411,7 +406,7 @@ export class FieldValueSelectionFilter extends Component< aria-label={config.name} options={items} renderOption={(option) => option.view} - isLoading={isNil(this.state.options)} + isLoading={isNil(options)} loadingMessage={config.loadingMessage} emptyMessage={config.noOptionsMessage} errorMessage={this.state.error} From 72dece8a1a4e1f88a3435f8c2d315b78c893d51d Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 29 Aug 2024 10:28:12 -0700 Subject: [PATCH 07/10] [tech debt] Remove unused class methods - appears to have been used before EuiSearchBar dogfooded EuiSelectable --- .../filters/field_value_selection_filter.tsx | 37 ------------------- 1 file changed, 37 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index 00b9907b230..9f16665e446 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -179,43 +179,6 @@ export class FieldValueSelectionFilter extends Component< }); } - filterOptions(q = '') { - this.setState((prevState) => { - if (isNil(prevState.options)) { - return {}; - } - - const predicate = this.getOptionFilter(); - - return { - ...prevState, - options: { - ...prevState.options, - shown: prevState.options.all.filter((option, i, options) => { - const name = this.resolveOptionName(option).toLowerCase(); - const query = q.toLowerCase(); - return predicate(name, query, options); - }), - }, - }; - }); - } - - getOptionFilter(): OptionsFilter { - const filterWith = - this.props.config.filterWith || defaults.config.filterWith; - - if (typeof filterWith === 'function') { - return filterWith; - } - - if (filterWith === 'includes') { - return (name, query) => name.includes(query); - } - - return (name, query) => name.startsWith(query); - } - resolveOptionsLoader: () => OptionsLoader = () => { const options = this.props.config.options; if (isArray(options)) { From 96d5b68ffb4211fba2c38d1842c2664a0a3a3489 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 29 Aug 2024 16:23:10 -0700 Subject: [PATCH 08/10] Various minor syntax/nit cleanups - store the count of activeItems instead of a full array, since we only care about the length - use getters and modern nullish syntax for configs with defaults --- .../filters/field_value_selection_filter.tsx | 42 ++++++++----------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index 9f16665e446..40c1c259f91 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -80,7 +80,7 @@ interface State { sorted: FieldValueOptionType[]; } | null; cachedOptions?: FieldValueOptionType[] | null; - activeItems: FieldValueOptionType[]; + activeItemsCount: number; } export class FieldValueSelectionFilter extends Component< @@ -101,7 +101,7 @@ export class FieldValueSelectionFilter extends Component< popoverOpen: false, error: null, options: preloadedOptions, - activeItems: [], + activeItemsCount: 0, }; } @@ -142,14 +142,12 @@ export class FieldValueSelectionFilter extends Component< const { query, config } = this.props; - const multiSelect = this.resolveMultiSelect(); - if (options) { options.forEach((op) => { const optionField = op.field || config.field; if (optionField) { const clause = - multiSelect === 'or' + this.multiSelect === 'or' ? query.getOrFieldClause(optionField, op.value) : query.getSimpleFieldClause(optionField, op.value); const checked = this.resolveChecked(clause); @@ -167,7 +165,7 @@ export class FieldValueSelectionFilter extends Component< this.setState({ error: null, - activeItems: items.on, + activeItemsCount: items.on.length, options: { unsorted: options, sorted: [...items.on, ...items.off, ...items.rest], @@ -215,7 +213,6 @@ export class FieldValueSelectionFilter extends Component< value: Value, checked?: Omit ) { - const multiSelect = this.resolveMultiSelect(); const { config: { autoClose, operator = Operator.EQ }, } = this.props; @@ -223,12 +220,12 @@ export class FieldValueSelectionFilter extends Component< // If the consumer explicitly sets `autoClose`, always defer to that. // Otherwise, default to auto-closing for single selections and leaving the // popover open for multi-select (so users can continue selecting options) - const shouldClosePopover = autoClose ?? !multiSelect; + const shouldClosePopover = autoClose ?? !this.multiSelect; if (shouldClosePopover) { this.closePopover(); } - if (!multiSelect) { + if (!this.multiSelect) { const query = checked ? this.props.query .removeSimpleFieldClauses(field) @@ -236,7 +233,7 @@ export class FieldValueSelectionFilter extends Component< : this.props.query.removeSimpleFieldClauses(field); this.props.onChange(query); - } else if (multiSelect === 'or') { + } else if (this.multiSelect === 'or') { const query = checked ? this.props.query.addOrFieldValue(field, value, true, operator) : this.props.query.removeOrFieldValue(field, value); @@ -251,11 +248,12 @@ export class FieldValueSelectionFilter extends Component< } } - resolveMultiSelect(): MultiSelect { - const { config } = this.props; - return !isNil(config.multiSelect) - ? config.multiSelect - : defaults.config.multiSelect; + get autoSortOptions() { + return this.props.config.autoSortOptions ?? defaults.config.autoSortOptions; + } + + get multiSelect(): MultiSelect { + return this.props.config.multiSelect ?? defaults.config.multiSelect; } componentDidMount() { @@ -268,11 +266,8 @@ export class FieldValueSelectionFilter extends Component< render() { const { query, config } = this.props; - const multiSelect = this.resolveMultiSelect(); - const autoSortOptions = - config.autoSortOptions ?? defaults.config.autoSortOptions; - const options = autoSortOptions + const options = this.autoSortOptions ? this.state.options?.sorted : this.state.options?.unsorted; @@ -281,7 +276,7 @@ export class FieldValueSelectionFilter extends Component< ? options.some((item) => this.isActiveField(item.field)) : false; - const activeItemsCount = this.state.activeItems.length; + const { activeItemsCount } = this.state; const active = (activeTop || activeItem) && activeItemsCount > 0; const button = ( @@ -308,7 +303,7 @@ export class FieldValueSelectionFilter extends Component< } const clause = - multiSelect === 'or' + this.multiSelect === 'or' ? query.getOrFieldClause(optionField, option.value) : query.getSimpleFieldClause(optionField, option.value); @@ -365,7 +360,7 @@ export class FieldValueSelectionFilter extends Component< }} > > - singleSelection={!multiSelect} + singleSelection={!this.multiSelect} aria-label={config.name} options={items} renderOption={(option) => option.view} @@ -415,9 +410,8 @@ export class FieldValueSelectionFilter extends Component< } const { query } = this.props; - const multiSelect = this.resolveMultiSelect(); - if (multiSelect === 'or') { + if (this.multiSelect === 'or') { return query.hasOrFieldClause(field); } From 0733b47827fb9d2cbcc7ee20b76862f7a1335030 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 29 Aug 2024 17:54:41 -0700 Subject: [PATCH 09/10] [tech debt] Convert Promise syntax to modern async/await - remove separate async `resolveOptionsLoader` fn - for some reason this separate method causes the EuiSelectableList to remount completely, no idea why :T + add missing clearTimeout for scheduled cache wipe, which could potentially cause stale errors after unmount + add missing test case for `cache` config --- .../field_value_selection_filter.spec.tsx | 62 +++++++++ .../filters/field_value_selection_filter.tsx | 131 ++++++++---------- 2 files changed, 123 insertions(+), 70 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx index 962322d4e12..ee42c9ef8fc 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx @@ -502,4 +502,66 @@ describe('FieldValueSelectionFilter', () => { .eq(0) .should('have.attr', 'title', 'Bug'); }); + + it('caches options if options is a function and config.cache is set', () => { + // Note: cy.clock()/cy.tick() doesn't currently work in Cypress component testing :T + // We should use that instead of cy.wait once https://github.com/cypress-io/cypress/issues/28846 is fixed + const props: FieldValueSelectionFilterProps = { + index: 0, + onChange: () => {}, + query: Query.parse(''), + config: { + type: 'field_value_selection', + field: 'tag', + name: 'Tag', + cache: 5000, // Cache the loaded tags for 5 seconds + options: () => + new Promise((resolve) => { + setTimeout(() => { + resolve(staticOptions); + }, 1000); // Spoof 1 second load time + }), + }, + }; + cy.spy(props.config, 'options'); + + const reducedTimeout = { timeout: 10 }; + const assertIsLoading = (expected?: Function) => { + cy.get('.euiSelectableListItem', reducedTimeout).should('have.length', 0); + cy.get('[data-test-subj="euiSelectableMessage"]', reducedTimeout) + .should('have.text', 'Loading options') + .then(() => { + expected?.(); + }); + }; + const assertIsLoaded = (expected?: Function) => { + cy.get('.euiSelectableListItem', reducedTimeout).should('have.length', 3); + cy.get('[data-test-subj="euiSelectableMessage"]', reducedTimeout) + .should('not.exist') + .then(() => { + expected?.(); + }); + }; + + cy.mount(); + cy.get('button').click(); + assertIsLoading(); + + // Wait out the async options loader + cy.wait(1000); + assertIsLoaded(() => expect(props.config.options).to.be.calledOnce); + + // Close and re-open the popover + cy.get('button').click(); + cy.get('button').click(); + + // Cached options should immediately repopulate + assertIsLoaded(() => expect(props.config.options).to.be.calledOnce); + + // Wait out the remainder of the cache, loading state should initiate again + cy.get('button').click(); + cy.wait(5000); + cy.get('button').click(); + assertIsLoading(() => expect(props.config.options).to.be.calledTwice); + }); }); diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index 40c1c259f91..da21b2041ce 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -87,6 +87,8 @@ export class FieldValueSelectionFilter extends Component< FieldValueSelectionFilterProps, State > { + cacheTimeout: ReturnType | undefined; + constructor(props: FieldValueSelectionFilterProps) { super(props); const { options } = props.config; @@ -125,83 +127,68 @@ export class FieldValueSelectionFilter extends Component< }); } - loadOptions() { - const loader = this.resolveOptionsLoader(); + loadOptions = async () => { + let loadedOptions: FieldValueOptionType[]; this.setState({ options: null, error: null }); - loader() - .then((options) => { - const items: { - on: FieldValueOptionType[]; - off: FieldValueOptionType[]; - rest: FieldValueOptionType[]; - } = { - on: [], - off: [], - rest: [], - }; - - const { query, config } = this.props; - - if (options) { - options.forEach((op) => { - const optionField = op.field || config.field; - if (optionField) { - const clause = - this.multiSelect === 'or' - ? query.getOrFieldClause(optionField, op.value) - : query.getSimpleFieldClause(optionField, op.value); - const checked = this.resolveChecked(clause); - if (!checked) { - items.rest.push(op); - } else if (checked === 'on') { - items.on.push(op); - } else { - items.off.push(op); - } - } - return; - }); - } - this.setState({ - error: null, - activeItemsCount: items.on.length, - options: { - unsorted: options, - sorted: [...items.on, ...items.off, ...items.rest], - }, - }); - }) - .catch(() => { - this.setState({ options: null, error: 'Could not load options' }); - }); - } - - resolveOptionsLoader: () => OptionsLoader = () => { - const options = this.props.config.options; - if (isArray(options)) { - return () => Promise.resolve(options); + const { options, cache } = this.props.config; + try { + if (isArray(options)) { + // Synchronous options, already loaded + loadedOptions = options; + } else { + // Async options loader fn, potentially with a cache + loadedOptions = this.state.cachedOptions ?? (await options()); + + // If a cache time is set, populate the cache and schedule a cache reset + if (cache != null && cache > 0) { + this.setState({ cachedOptions: loadedOptions }); + this.cacheTimeout = setTimeout(() => { + this.setState({ cachedOptions: null }); + }, cache); + } + } + } catch { + return this.setState({ options: null, error: 'Could not load options' }); } - return () => { - const cachedOptions = this.state.cachedOptions; - if (cachedOptions) { - return Promise.resolve(cachedOptions); - } + const items: Record = { + on: [], + off: [], + rest: [], + }; - return (options as OptionsLoader)().then((opts) => { - // If a cache time is set, populate the cache and also schedule a - // cache reset. - if (this.props.config.cache != null && this.props.config.cache > 0) { - this.setState({ cachedOptions: opts }); - setTimeout(() => { - this.setState({ cachedOptions: null }); - }, this.props.config.cache); - } + const { query, config } = this.props; - return opts; + if (loadedOptions) { + loadedOptions.forEach((op) => { + const optionField = op.field || config.field; + if (optionField) { + const clause = + this.multiSelect === 'or' + ? query.getOrFieldClause(optionField, op.value) + : query.getSimpleFieldClause(optionField, op.value); + const checked = this.resolveChecked(clause); + if (!checked) { + items.rest.push(op); + } else if (checked === 'on') { + items.on.push(op); + } else { + items.off.push(op); + } + } + return; }); - }; + } + + this.setState({ + error: null, + activeItemsCount: items.on.length, + options: { + unsorted: loadedOptions, + sorted: [...items.on, ...items.off, ...items.rest], + }, + }); }; resolveOptionName(option: FieldValueOptionType) { @@ -264,6 +251,10 @@ export class FieldValueSelectionFilter extends Component< if (this.props.query !== prevProps.query) this.loadOptions(); } + componentWillUnmount() { + clearTimeout(this.cacheTimeout); + } + render() { const { query, config } = this.props; From c4972d264cee70d5f23736fdb6ec0bf8aaae960e Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 29 Aug 2024 18:52:04 -0700 Subject: [PATCH 10/10] Restore UX where auto-sorted items should be scrolled to after being checked + retain selected/active index highlight (better UX than before, where focus/active index was lost) + simplify stateful test components and update autosort tests to be simpler and check for active attributes --- .../field_value_selection_filter.spec.tsx | 132 ++++++------------ .../filters/field_value_selection_filter.tsx | 44 ++++-- 2 files changed, 79 insertions(+), 97 deletions(-) diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx index ee42c9ef8fc..7f1f3464bea 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.spec.tsx @@ -15,6 +15,7 @@ import { requiredProps } from '../../../test'; import { FieldValueSelectionFilter, FieldValueSelectionFilterProps, + FieldValueSelectionFilterConfigType, } from './field_value_selection_filter'; import { Query } from '../query'; @@ -34,6 +35,29 @@ const staticOptions = [ ]; describe('FieldValueSelectionFilter', () => { + const FieldValueSelectionFilterWithState = ( + config: Partial + ) => { + const [query, setQuery] = useState(Query.parse('')); + const onChange = (newQuery: Query) => setQuery(newQuery); + + const props: FieldValueSelectionFilterProps = { + ...requiredProps, + index: 0, + onChange, + query, + config: { + type: 'field_value_selection', + field: 'tag', + name: 'Tag', + options: staticOptions, + ...config, + }, + }; + + return ; + }; + it('allows options as a function', () => { const props: FieldValueSelectionFilterProps = { ...requiredProps, @@ -140,31 +164,6 @@ describe('FieldValueSelectionFilter', () => { }); describe('multi-select testing', () => { - const FieldValueSelectionFilterWithState = ({ - multiSelect, - }: { - multiSelect: 'or' | boolean; - }) => { - const [query, setQuery] = useState(Query.parse('')); - const onChange = (newQuery: Query) => setQuery(newQuery); - - const props: FieldValueSelectionFilterProps = { - ...requiredProps, - index: 0, - onChange, - query, - config: { - type: 'field_value_selection', - field: 'tag', - name: 'Tag', - multiSelect, - options: staticOptions, - }, - }; - - return ; - }; - it('uses multi-select OR', () => { cy.mount(); cy.get('button').click(); @@ -226,33 +225,6 @@ describe('FieldValueSelectionFilter', () => { }); describe('auto-close testing', () => { - const FieldValueSelectionFilterWithState = ({ - autoClose, - multiSelect, - }: { - autoClose: undefined | boolean; - multiSelect: 'or' | boolean; - }) => { - const [query, setQuery] = useState(Query.parse('')); - const onChange = (newQuery: Query) => setQuery(newQuery); - - const props: FieldValueSelectionFilterProps = { - ...requiredProps, - index: 0, - onChange, - query, - config: { - type: 'field_value_selection', - field: 'tag', - name: 'Tag', - multiSelect, - autoClose, - options: staticOptions, - }, - }; - - return ; - }; const selectFilter = () => { // Open popover cy.get('button').click(); @@ -339,51 +311,33 @@ describe('FieldValueSelectionFilter', () => { }); describe('autoSortOptions', () => { - it('sorts selected options to the top by default', () => { - const props: FieldValueSelectionFilterProps = { - index: 0, - onChange: () => {}, - query: Query.parse('tag:bug'), - config: { - type: 'field_value_selection', - field: 'tag', - name: 'Tag', - options: staticOptions, - }, - }; - cy.mount(); - cy.get('.euiNotificationBadge').should('exist'); + const getOptions = () => cy.get('.euiSelectableListItem'); + it('sorts selected options to the top by default', () => { + cy.mount(); cy.get('button').click(); - const getOptions = () => cy.get('.euiSelectableListItem'); - getOptions().should('have.length', 3); - getOptions().eq(0).should('have.attr', 'title', 'Bug'); - getOptions().eq(1).should('have.attr', 'title', 'feature'); + + getOptions().last().should('have.attr', 'title', 'Bug').click(); + // Should have moved to the top of the list and retained active focus + getOptions() + .first() + .should('have.attr', 'title', 'Bug') + .should('have.attr', 'aria-checked', 'true') + .should('have.attr', 'aria-selected', 'true'); }); it('does not sort selected options to the top when set to false', () => { - const props: FieldValueSelectionFilterProps = { - index: 0, - onChange: () => {}, - query: Query.parse('tag:bug'), - config: { - type: 'field_value_selection', - field: 'tag', - name: 'Tag', - options: staticOptions, - autoSortOptions: false, - }, - }; - cy.mount(); - cy.get('.euiNotificationBadge').should('exist'); - + cy.mount(); cy.get('button').click(); - const getOptions = () => cy.get('.euiSelectableListItem'); - getOptions().should('have.length', 3); - getOptions().eq(2).should('have.attr', 'title', 'Bug'); - getOptions().eq(0).should('have.attr', 'title', 'feature'); + + getOptions().last().should('have.attr', 'title', 'Bug').click(); + getOptions() + .last() + .should('have.attr', 'title', 'Bug') + .should('have.attr', 'aria-checked', 'true') + .should('have.attr', 'aria-selected', 'true'); }); }); diff --git a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx index da21b2041ce..152f5b66c6b 100644 --- a/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/packages/eui/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import React, { Component, ReactNode } from 'react'; +import React, { Component, ReactNode, createRef } from 'react'; import { RenderWithEuiTheme } from '../../../services'; import { isArray, isNil } from '../../../services/predicate'; @@ -81,12 +81,14 @@ interface State { } | null; cachedOptions?: FieldValueOptionType[] | null; activeItemsCount: number; + lastCheckedValue?: Value; } export class FieldValueSelectionFilter extends Component< FieldValueSelectionFilterProps, State > { + selectableClassRef = createRef(); cacheTimeout: ReturnType | undefined; constructor(props: FieldValueSelectionFilterProps) { @@ -181,14 +183,35 @@ export class FieldValueSelectionFilter extends Component< }); } - this.setState({ - error: null, - activeItemsCount: items.on.length, - options: { - unsorted: loadedOptions, - sorted: [...items.on, ...items.off, ...items.rest], + this.setState( + { + error: null, + activeItemsCount: items.on.length, + options: { + unsorted: loadedOptions, + sorted: [...items.on, ...items.off, ...items.rest], + }, }, - }); + this.scrollToAutoSortedOption + ); + }; + + scrollToAutoSortedOption = () => { + if (!this.autoSortOptions) return; + + const { lastCheckedValue, options } = this.state; + if (lastCheckedValue) { + const sortedIndex = options!.sorted.findIndex( + (option) => option.value === lastCheckedValue + ); + if (sortedIndex >= 0) { + // EuiSelectable should automatically handle scrolling its list to the new index + this.selectableClassRef.current?.setState({ + activeOptionIndex: sortedIndex, + }); + } + this.setState({ lastCheckedValue: undefined }); + } }; resolveOptionName(option: FieldValueOptionType) { @@ -204,6 +227,10 @@ export class FieldValueSelectionFilter extends Component< config: { autoClose, operator = Operator.EQ }, } = this.props; + if (checked && this.autoSortOptions) { + this.setState({ lastCheckedValue: value }); + } + // If the consumer explicitly sets `autoClose`, always defer to that. // Otherwise, default to auto-closing for single selections and leaving the // popover open for multi-select (so users can continue selecting options) @@ -351,6 +378,7 @@ export class FieldValueSelectionFilter extends Component< }} > > + ref={this.selectableClassRef} singleSelection={!this.multiSelect} aria-label={config.name} options={items}