From 8f370b038da073c9431f654e9dbfdf46661ef321 Mon Sep 17 00:00:00 2001 From: Eric Peterson Date: Sat, 22 Jan 2022 18:05:21 +0100 Subject: [PATCH] Review feedback. Signed-off-by: Eric Peterson --- .changeset/search-something-new.md | 14 +++--- .changeset/search-under-the-sun.md | 2 +- .../app/src/components/search/SearchPage.tsx | 12 ++--- .../app/src/components/search/SearchPage.tsx | 10 ++-- plugins/search/api-report.md | 8 +-- .../SearchFilter.Autocomplete.test.tsx | 49 ++++++++---------- .../SearchFilter.Autocomplete.tsx | 20 +++++--- .../SearchFilter/SearchFilter.stories.tsx | 18 ++++--- .../SearchFilter/SearchFilter.test.tsx | 2 +- .../components/SearchFilter/SearchFilter.tsx | 50 +++++++++++++------ .../src/components/SearchFilter/hooks.ts | 19 ++++--- 11 files changed, 115 insertions(+), 89 deletions(-) diff --git a/.changeset/search-something-new.md b/.changeset/search-something-new.md index 260d1055a4..58a817e3ad 100644 --- a/.changeset/search-something-new.md +++ b/.changeset/search-something-new.md @@ -2,7 +2,7 @@ '@backstage/create-app': patch --- -An example instance of the new `` was added to the composed `SearchPage.tsx`, allowing searches bound to the `techdocs` type to be filtered by entity name. +An example instance of a `` with asynchronously loaded values was added to the composed `SearchPage.tsx`, allowing searches bound to the `techdocs` type to be filtered by entity name. This is an entirely optional change; if you wish to adopt it, you can make the following (or similar) changes to your search page layout: @@ -50,12 +50,12 @@ This is an entirely optional change; if you wish to adopt it, you can make the f /> + {types.includes('techdocs') && ( -+ { -+ // Return a list of entitis which are documented. ++ values={async () => { ++ // Return a list of entities which are documented. + const { items } = await catalogApi.getEntities({ + fields: ['metadata.name'], + filter: { @@ -64,9 +64,9 @@ This is an entirely optional change; if you wish to adopt it, you can make the f + }, + }); + -+ return items -+ .map(entity => entity.metadata.name) -+ .filter(name => name.includes(partial)); ++ const names = items.map(entity => entity.metadata.name); ++ names.sort(); ++ return names; + }} + /> + )} diff --git a/.changeset/search-under-the-sun.md b/.changeset/search-under-the-sun.md index 2cf5d46cdb..3f97a60c1c 100644 --- a/.changeset/search-under-the-sun.md +++ b/.changeset/search-under-the-sun.md @@ -4,6 +4,6 @@ Introduces a `` variant, which can be used as either a single- or multi-select autocomplete filter. -This variant, as well as ``, now also supports loading allowed values asynchronously with a new `asyncValues` prop, which takes an asynchronous function that resolves to the list of values (an optional `asyncDebounce` prop may also be provided). +This variant, as well as ``, now also supports loading allowed values asynchronously by passing a function that resolves the list of values to the `values` prop. (An optional `valuesDebounceMs` prop may also be provided to control the debounce time). Check the [search plugin storybook](https://backstage.io/storybook/?path=/story/plugins-search-searchfilter) to see how to leverage these new additions. diff --git a/packages/app/src/components/search/SearchPage.tsx b/packages/app/src/components/search/SearchPage.tsx index 3b839f9b1f..d4e60244d7 100644 --- a/packages/app/src/components/search/SearchPage.tsx +++ b/packages/app/src/components/search/SearchPage.tsx @@ -93,12 +93,12 @@ const SearchPage = () => { /> {types.includes('techdocs') && ( - { - // Return a list of entitis which are documented. + values={async () => { + // Return a list of entities which are documented. const { items } = await catalogApi.getEntities({ fields: ['metadata.name'], filter: { @@ -107,9 +107,9 @@ const SearchPage = () => { }, }); - return items - .map(entity => entity.metadata.name) - .filter(name => name.includes(partial)); + const names = items.map(entity => entity.metadata.name); + names.sort(); + return names; }} /> )} diff --git a/packages/create-app/templates/default-app/packages/app/src/components/search/SearchPage.tsx b/packages/create-app/templates/default-app/packages/app/src/components/search/SearchPage.tsx index d9508e24e8..a88e7250e9 100644 --- a/packages/create-app/templates/default-app/packages/app/src/components/search/SearchPage.tsx +++ b/packages/create-app/templates/default-app/packages/app/src/components/search/SearchPage.tsx @@ -74,11 +74,11 @@ const SearchPage = () => { /> {types.includes('techdocs') && ( - { + values={async () => { // Return a list of entities which are documented. const { items } = await catalogApi.getEntities({ fields: ['metadata.name'], @@ -88,9 +88,9 @@ const SearchPage = () => { }, }); - return items - .map(entity => entity.metadata.name) - .filter(name => name.includes(partial)); + const names = items.map(entity => entity.metadata.name); + names.sort(); + return names; }} /> )} diff --git a/plugins/search/api-report.md b/plugins/search/api-report.md index b63271b94d..fc82e2be47 100644 --- a/plugins/search/api-report.md +++ b/plugins/search/api-report.md @@ -94,8 +94,9 @@ export const searchApiRef: ApiRef; // @public (undocumented) export type SearchAutocompleteFilterProps = SearchFilterComponentProps & { - multiple?: boolean; + filterSelectedOptions?: boolean; limitTags?: number; + multiple?: boolean; }; // @public @@ -166,10 +167,9 @@ export type SearchFilterComponentProps = { className?: string; name: string; label?: string; - values?: string[]; - asyncValues?: (partial: string) => Promise; - asyncDebounce?: number; + values?: string[] | ((partial: string) => Promise); defaultValue?: string[] | string | null; + valuesDebounceMs?: number; }; // Warning: (ae-missing-release-tag) "SearchFilterNext" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.test.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.test.tsx index 7a8bfe2624..5c774c0d2f 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.test.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.test.tsx @@ -14,8 +14,7 @@ * limitations under the License. */ -import { ApiProvider } from '@backstage/core-app-api'; -import { TestApiRegistry } from '@backstage/test-utils'; +import { TestApiProvider } from '@backstage/test-utils'; import { screen, render, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; @@ -35,7 +34,6 @@ const SearchContextFilterSpy = ({ name }: { name: string }) => { describe('SearchFilter.Autocomplete', () => { const query = jest.fn().mockResolvedValue({}); - const mockApis = TestApiRegistry.from([searchApiRef, { query }]); const emptySearchContext = { term: '', types: [], @@ -47,11 +45,11 @@ describe('SearchFilter.Autocomplete', () => { it('renders as expected', async () => { render( - + - , + , ); const autocomplete = screen.getByRole('combobox'); @@ -68,14 +66,11 @@ describe('SearchFilter.Autocomplete', () => { it('renders as expected with async values', async () => { render( - + - values} - /> + values} /> - , + , ); const autocomplete = screen.getByRole('combobox'); @@ -92,7 +87,7 @@ describe('SearchFilter.Autocomplete', () => { it('does not affect unrelated filter state', async () => { render( - + { - , + , ); // The spy should show the initial value. @@ -136,7 +131,7 @@ describe('SearchFilter.Autocomplete', () => { describe('single', () => { it('renders as expected with defaultValue', async () => { render( - + { /> - , + , ); const autocomplete = screen.getByRole('combobox'); @@ -161,7 +156,7 @@ describe('SearchFilter.Autocomplete', () => { it('renders as expected with initial context', async () => { render( - + { - , + , ); const autocomplete = screen.getByRole('combobox'); @@ -187,12 +182,12 @@ describe('SearchFilter.Autocomplete', () => { it('sets filter state when selecting a value', async () => { render( - + - , + , ); // Select the first option in the autocomplete. @@ -225,7 +220,7 @@ describe('SearchFilter.Autocomplete', () => { describe('multiple', () => { it('renders as expected with defaultValue', async () => { render( - + { /> - , + , ); await waitFor(() => { @@ -249,7 +244,7 @@ describe('SearchFilter.Autocomplete', () => { it('renders as expected with initial context', async () => { render( - + { - , + , ); await waitFor(() => { @@ -273,7 +268,7 @@ describe('SearchFilter.Autocomplete', () => { it('respects tag limit configuration', async () => { render( - + { values={values} /> - , + , ); const autocomplete = screen.getByRole('combobox'); @@ -322,12 +317,12 @@ describe('SearchFilter.Autocomplete', () => { it('sets filter state when selecting a value', async () => { render( - + - , + , ); const autocomplete = screen.getByRole('combobox'); diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.tsx index b329cf3b03..9a27f63565 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.Autocomplete.tsx @@ -29,29 +29,34 @@ import { SearchFilterComponentProps } from './SearchFilter'; * @public */ export type SearchAutocompleteFilterProps = SearchFilterComponentProps & { - multiple?: boolean; + filterSelectedOptions?: boolean; limitTags?: number; + multiple?: boolean; }; export const AutocompleteFilter = (props: SearchAutocompleteFilterProps) => { const { - asyncValues, - asyncDebounce, className, defaultValue, - multiple, name, values: givenValues, + valuesDebounceMs, label, + filterSelectedOptions, limitTags, + multiple, } = props; const [inputValue, setInputValue] = useState(''); useDefaultFilterValue(name, defaultValue); + const asyncValues = + typeof givenValues === 'function' ? givenValues : undefined; + const defaultValues = + typeof givenValues === 'function' ? undefined : givenValues; const { value: values, loading } = useAsyncFilterValues( asyncValues, inputValue, - givenValues, - asyncDebounce, + defaultValues, + valuesDebounceMs, ); const { filters, setFilters } = useSearch(); const filterValue = @@ -94,12 +99,13 @@ export const AutocompleteFilter = (props: SearchAutocompleteFilterProps) => { return ( setInputValue(newValue)} diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.stories.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.stories.tsx index e2a73a42bc..c43753e8d2 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.stories.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.stories.tsx @@ -50,7 +50,8 @@ export const SelectFilter = () => { return ( @@ -61,8 +62,9 @@ export const AsyncSelectFilter = () => { return ( { + label="Asynchronous Values" + name="async_values" + values={async () => { const response = await fetch('https://swapi.dev/api/planets'); const json: { results: Array<{ name: string }> } = await response.json(); @@ -79,7 +81,7 @@ export const Autocomplete = () => { ); @@ -92,7 +94,7 @@ export const MultiSelectAutocomplete = () => { multiple name="autocomplete" label="Multi-Select Autocomplete Filter" - values={['abba', 'cadaver']} + values={['value1', 'value2']} /> ); @@ -105,10 +107,12 @@ export const AsyncMultiSelectAutocomplete = () => { multiple name="starwarsPerson" label="Starwars Character" - asyncValues={async partial => { + values={async partial => { if (partial === '') return []; const response = await fetch( - `https://swapi.dev/api/people?search=${partial}`, + `https://swapi.dev/api/people?search=${encodeURIComponent( + partial, + )}`, ); const json: { results: Array<{ name: string }> } = await response.json(); diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx index 31539df0d8..7b997088ec 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.test.tsx @@ -215,7 +215,7 @@ describe('SearchFilter', () => { values} + values={async () => values} /> , ); diff --git a/plugins/search/src/components/SearchFilter/SearchFilter.tsx b/plugins/search/src/components/SearchFilter/SearchFilter.tsx index 534db60130..1308333100 100644 --- a/plugins/search/src/components/SearchFilter/SearchFilter.tsx +++ b/plugins/search/src/components/SearchFilter/SearchFilter.tsx @@ -46,18 +46,19 @@ export type SearchFilterComponentProps = { className?: string; name: string; label?: string; - values?: string[]; /** - * An async function to return a list of values to be used in the filter. In - * the autocomplete filter, the last input value is provided as an input to - * allow values to be filtered. This function is debounced and values cached. + * Either an array of values directly, or an async function to return a list + * of values to be used in the filter. In the autocomplete filter, the last + * input value is provided as an input to allow values to be filtered. This + * function is debounced and values cached. */ - asyncValues?: (partial: string) => Promise; - /** - * Debounce time (ms) used by the asyncValues callback. Defaults to 250ms. - */ - asyncDebounce?: number; + values?: string[] | ((partial: string) => Promise); defaultValue?: string[] | string | null; + /** + * Debounce time in milliseconds, used when values is an async callback. + * Defaults to 250ms. + */ + valuesDebounceMs?: number; }; /** @@ -69,10 +70,27 @@ export type SearchFilterWrapperProps = SearchFilterComponentProps & { }; const CheckboxFilter = (props: SearchFilterComponentProps) => { - const { className, defaultValue, label, name, values = [] } = props; + const { + className, + defaultValue, + label, + name, + values: givenValues = [], + valuesDebounceMs, + } = props; const classes = useStyles(); const { filters, setFilters } = useSearch(); useDefaultFilterValue(name, defaultValue); + const asyncValues = + typeof givenValues === 'function' ? givenValues : undefined; + const defaultValues = + typeof givenValues === 'function' ? undefined : givenValues; + const { value: values = [], loading } = useAsyncFilterValues( + asyncValues, + '', + defaultValues, + valuesDebounceMs, + ); const handleChange = (e: ChangeEvent) => { const { @@ -90,6 +108,7 @@ const CheckboxFilter = (props: SearchFilterComponentProps) => { return ( @@ -117,21 +136,24 @@ const CheckboxFilter = (props: SearchFilterComponentProps) => { const SelectFilter = (props: SearchFilterComponentProps) => { const { - asyncValues, - asyncDebounce, className, defaultValue, label, name, values: givenValues, + valuesDebounceMs, } = props; const classes = useStyles(); useDefaultFilterValue(name, defaultValue); + const asyncValues = + typeof givenValues === 'function' ? givenValues : undefined; + const defaultValues = + typeof givenValues === 'function' ? undefined : givenValues; const { value: values = [], loading } = useAsyncFilterValues( asyncValues, '', - givenValues, - asyncDebounce, + defaultValues, + valuesDebounceMs, ); const { filters, setFilters } = useSearch(); diff --git a/plugins/search/src/components/SearchFilter/hooks.ts b/plugins/search/src/components/SearchFilter/hooks.ts index 14dd00b8e7..7b27ac1fe2 100644 --- a/plugins/search/src/components/SearchFilter/hooks.ts +++ b/plugins/search/src/components/SearchFilter/hooks.ts @@ -29,7 +29,7 @@ export const useAsyncFilterValues = ( defaultValues: string[] = [], debounce: number = 250, ) => { - const valuesMemo = useRef>({}); + const valuesMemo = useRef>>({}); const definiteFn = fn || (() => Promise.resolve([])); const [state, callback] = useAsyncFn(definiteFn, [inputValue], { @@ -42,8 +42,10 @@ export const useAsyncFilterValues = ( // Performance optimization: only invoke the callback once per inputValue // for the lifetime of the hook/component. if (valuesMemo.current[inputValue] === undefined) { - callback(inputValue).then(values => { + valuesMemo.current[inputValue] = callback(inputValue).then(values => { + // Overrite the value for future immediate returns. valuesMemo.current[inputValue] = values; + return values; }); } }, @@ -59,11 +61,12 @@ export const useAsyncFilterValues = ( }; } - // Immediately return a memoized value if it is set. - if (valuesMemo.current[inputValue] !== undefined) { + // Immediately return a memoized value if it is set (and not a promise). + const possibleValue = valuesMemo.current[inputValue]; + if (Array.isArray(possibleValue)) { return { loading: false, - value: valuesMemo.current[inputValue], + value: possibleValue, }; } @@ -80,11 +83,7 @@ export const useDefaultFilterValue = ( const { setFilters } = useSearch(); useEffect(() => { - const defaultIsEmpty = !defaultValue; - const defaultIsEmptyArray = - Array.isArray(defaultValue) && defaultValue.length === 0; - - if (!defaultIsEmpty && !defaultIsEmptyArray) { + if (defaultValue && [defaultValue].flat().length > 0) { setFilters(prevFilters => ({ ...prevFilters, [name]: defaultValue,