-
Notifications
You must be signed in to change notification settings - Fork 15
feat(SearchField): WEB-2317 Search field empty suggestion and improvements #1473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
30bd881
e62e1f6
2a7ee19
8f3900b
a1b0cf8
1c21c49
ce05d90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import * as React from 'react'; | ||
| import {act, render, screen} from '@testing-library/react'; | ||
| import SearchField from '../search-field'; | ||
| import ThemeContextProvider from '../theme-context-provider'; | ||
| import {makeTheme} from './test-utils'; | ||
| import userEvent from '@testing-library/user-event'; | ||
|
|
||
| const getSuggestions = (value: string) => { | ||
| const allSuggestions = ['Apple', 'Banana', 'Orange']; | ||
| return allSuggestions.filter((item) => item.toLowerCase().includes(value.toLowerCase())); | ||
| }; | ||
|
|
||
| test('Show show suggestions on focus when shouldShowSuggestions is "focus"', async () => { | ||
| await act(async () => | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <SearchField | ||
| getSuggestions={getSuggestions} | ||
| shouldShowSuggestions="focus" | ||
| label="Search" | ||
| name="search" | ||
| /> | ||
| </ThemeContextProvider> | ||
| ) | ||
| ); | ||
|
|
||
| expect(screen.queryByRole('menuitem', {name: 'Apple'})).not.toBeInTheDocument(); | ||
|
|
||
| await userEvent.click(await screen.findByLabelText('Search')); | ||
|
|
||
| expect(await screen.findByRole('menuitem', {name: 'Apple'})).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('Should show suggestions on type when shouldShowSuggestions is 2', async () => { | ||
| await act(async () => | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <SearchField | ||
| getSuggestions={getSuggestions} | ||
| shouldShowSuggestions={2} | ||
| label="Search" | ||
| name="search" | ||
| /> | ||
| </ThemeContextProvider> | ||
| ) | ||
| ); | ||
|
|
||
| await userEvent.click(await screen.findByLabelText('Search')); | ||
| expect(screen.queryByRole('menuitem', {name: 'Apple'})).not.toBeInTheDocument(); | ||
|
|
||
| await userEvent.type(await screen.findByLabelText('Search'), 'A'); | ||
| expect(screen.queryByRole('menuitem', {name: 'Apple'})).not.toBeInTheDocument(); | ||
|
|
||
| await userEvent.type(await screen.findByLabelText('Search'), 'p'); | ||
| expect(await screen.findByRole('menuitem', {name: 'Apple'})).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('Should reload suggestions when clearing the field', async () => { | ||
| await act(async () => | ||
| render( | ||
| <ThemeContextProvider theme={makeTheme()}> | ||
| <SearchField | ||
| getSuggestions={getSuggestions} | ||
| shouldShowSuggestions="focus" | ||
| label="Search" | ||
| name="search" | ||
| /> | ||
| </ThemeContextProvider> | ||
| ) | ||
| ); | ||
|
|
||
| await userEvent.type(await screen.findByLabelText('Search'), 'invent'); | ||
|
|
||
| expect(screen.queryByRole('menuitem', {name: 'Apple'})).not.toBeInTheDocument(); | ||
|
|
||
| await userEvent.click(await screen.findByRole('button', {name: 'Borrar búsqueda'})); | ||
|
|
||
| expect(screen.getByRole('menuitem', {name: 'Apple'})).toBeInTheDocument(); | ||
| }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is my last pr with import sorting XD |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,26 @@ | ||
| 'use client'; | ||
| import * as React from 'react'; | ||
| import {useFieldProps} from './form-context'; | ||
| import {FieldEndIcon, TextFieldBaseAutosuggest} from './text-field-base'; | ||
| import IconSearchRegular from './generated/mistica-icons/icon-search-regular'; | ||
| import IconCloseRegular from './generated/mistica-icons/icon-close-regular'; | ||
| import IconSearchRegular from './generated/mistica-icons/icon-search-regular'; | ||
| import {useTheme} from './hooks'; | ||
| import {createChangeEvent} from './utils/dom'; | ||
| import {combineRefs} from './utils/common'; | ||
| import {iconSize} from './icon-button.css'; | ||
| import {FieldEndIcon, TextFieldBaseAutosuggest} from './text-field-base'; | ||
| import * as tokens from './text-tokens'; | ||
| import {combineRefs} from './utils/common'; | ||
| import {createChangeEvent} from './utils/dom'; | ||
|
|
||
| import type {CommonFormFieldProps} from './text-field-base'; | ||
|
|
||
| export interface SearchFieldProps extends CommonFormFieldProps { | ||
| onChangeValue?: (value: string, rawValue: string) => void; | ||
| getSuggestions?: (value: string) => ReadonlyArray<string>; | ||
| /** | ||
| * Indicates when suggestions should be shown. | ||
| * - 'focus': Show suggestions when the input is focused. | ||
| * - number: Show suggestions after a certain number of characters have been typed. | ||
| */ | ||
| shouldShowSuggestions?: 'focus' | number; | ||
| inputMode?: React.HTMLAttributes<HTMLInputElement>['inputMode']; | ||
| withStartIcon?: boolean; | ||
| } | ||
|
|
@@ -44,6 +50,7 @@ const SearchField = React.forwardRef<any, SearchFieldProps>( | |
| const {texts, t} = useTheme(); | ||
| const inputRef = React.useRef<HTMLInputElement>(null); | ||
| const [searchValue, setSearchValue] = React.useState(defaultValue || ''); | ||
| const didClearFieldRef = React.useRef(false); | ||
|
|
||
| const isControlledByParent = typeof value !== 'undefined'; | ||
|
|
||
|
|
@@ -60,13 +67,22 @@ const SearchField = React.forwardRef<any, SearchFieldProps>( | |
| ); | ||
|
|
||
| const clearInput = React.useCallback(() => { | ||
| didClearFieldRef.current = true; | ||
| handleChangeValue('', ''); | ||
| if (inputRef.current) { | ||
| onChange?.(createChangeEvent(inputRef.current, '')); | ||
| inputRef.current.focus(); | ||
| } | ||
| }, [handleChangeValue, onChange]); | ||
|
|
||
| React.useEffect(() => { | ||
| // When clearing the field, we need to blur and focus again the input so suggestions are reloaded | ||
| if (didClearFieldRef.current && controlledValue === '') { | ||
| didClearFieldRef.current = false; | ||
| inputRef?.current?.blur(); | ||
| inputRef?.current?.focus(); | ||
|
Comment on lines
+80
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't we execute this on clearInput callback instead of using an effect? const clearInput = React.useCallback(() => {
flushSync(() => {
handleChangeValue('', '');
if (inputRef.current) {
onChange?.(createChangeEvent(inputRef.current, ''));
}
});
inputRef.current?.blur();
inputRef.current?.focus();
}, [handleChangeValue, onChange]); |
||
| } | ||
| }, [controlledValue]); | ||
|
|
||
| const fieldProps = useFieldProps({ | ||
| name, | ||
| label, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| import {globalStyle, style} from '@vanilla-extract/css'; | ||
| import {sprinkles} from './sprinkles.css'; | ||
| import {vars} from './skins/skin-contract.css'; | ||
| import {iconContainerSize} from './icon-button.css'; | ||
| import * as mq from './media-queries.css'; | ||
| import {vars as skinVars, vars} from './skins/skin-contract.css'; | ||
| import {sprinkles} from './sprinkles.css'; | ||
| import {pxToRem} from './utils/css'; | ||
| import {iconContainerSize} from './icon-button.css'; | ||
|
|
||
| const borderSize = 1; | ||
|
|
||
|
|
@@ -328,6 +328,18 @@ export const prefix = style([ | |
| }, | ||
| ]); | ||
|
|
||
| export const emptyCase = style([ | ||
| sprinkles({ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| }), | ||
| { | ||
| height: pxToRem(48), | ||
| padding: '6px 16px', | ||
| userSelect: 'none', | ||
| }, | ||
| ]); | ||
|
|
||
| export const menuItem = style([ | ||
| sprinkles({ | ||
| display: 'flex', | ||
|
|
@@ -337,27 +349,34 @@ export const menuItem = style([ | |
| { | ||
| height: pxToRem(48), | ||
| padding: '6px 16px', | ||
| transition: 'background-color 150ms cubic-bezier(0.4, 0, 0.2, 1) 0ms', | ||
| selectors: { | ||
| '&:hover': { | ||
| background: 'rgba(0, 0, 0, 0.08)', | ||
| }, | ||
| }, | ||
| transition: 'background-color 0.1s ease-in-out', | ||
| }, | ||
| ]); | ||
|
|
||
| export const menuItemSelected = sprinkles({ | ||
| background: vars.colors.backgroundAlternative, | ||
| export const menuItemSelected = style({ | ||
| backgroundColor: skinVars.colors.backgroundContainerHover, | ||
| ':active': { | ||
| backgroundColor: skinVars.colors.backgroundContainerPressed, | ||
| }, | ||
| '@media': { | ||
| [mq.touchableOnly]: { | ||
| backgroundColor: 'transparent', | ||
| transition: 'none', | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| export const suggestionsContainer = style([ | ||
| sprinkles({ | ||
| position: 'absolute', | ||
| }), | ||
| { | ||
| boxShadow: | ||
| '0px 2px 1px -1px rgba(0,0,0,0.2), 0px 1px 1px 0px rgba(0,0,0,0.14), 0px 1px 3px 0px rgba(0,0,0,0.12)', | ||
| background: 'white', | ||
| marginTop: 8, | ||
| boxSizing: 'border-box', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Easier to set a fixed width without having to take padding in account |
||
| boxShadow: '0px 2px 4px 0px #00000033', | ||
| padding: 8, | ||
| background: skinVars.colors.backgroundContainer, | ||
| borderRadius: skinVars.borderRadii.popup, | ||
|
|
||
| // one more than TextField label | ||
| zIndex: 2, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,22 @@ | ||
| 'use client'; | ||
| import * as React from 'react'; | ||
| import {Label, HelperText, FieldContainer} from './text-field-components'; | ||
| import {LABEL_SCALE_MOBILE, LABEL_SCALE_DESKTOP} from './text-field-components.css'; | ||
| import {Text3} from './text'; | ||
| import {isRunningAcceptanceTest, isFirefox} from './utils/platform'; | ||
| import {useTheme, useScreenSize, useIsomorphicLayoutEffect} from './hooks'; | ||
| import classNames from 'classnames'; | ||
| import {combineRefs} from './utils/common'; | ||
| import * as styles from './text-field-base.css'; | ||
| import {vars} from './skins/skin-contract.css'; | ||
| import * as React from 'react'; | ||
| import {useIsomorphicLayoutEffect, useScreenSize, useTheme} from './hooks'; | ||
| import {InternalIconButton, InternalToggleIconButton} from './icon-button'; | ||
| import {ThemeVariant} from './theme-variant-context'; | ||
| import {iconSize} from './icon-button.css'; | ||
| import {vars} from './skins/skin-contract.css'; | ||
| import {Text3} from './text'; | ||
| import * as styles from './text-field-base.css'; | ||
| import {FieldContainer, HelperText, Label} from './text-field-components'; | ||
| import {LABEL_SCALE_DESKTOP, LABEL_SCALE_MOBILE} from './text-field-components.css'; | ||
| import * as tokens from './text-tokens'; | ||
| import {ThemeVariant} from './theme-variant-context'; | ||
| import {combineRefs} from './utils/common'; | ||
| import {isFirefox, isRunningAcceptanceTest} from './utils/platform'; | ||
|
|
||
| import type {DataAttributes, IconProps} from './utils/types'; | ||
| import type {InputState} from './text-field-components'; | ||
| import type {FieldValidator} from './form-context'; | ||
| import type {InputState} from './text-field-components'; | ||
| import type {DataAttributes, IconProps} from './utils/types'; | ||
| import type {ExclusifyUnion} from './utils/utility-types'; | ||
|
|
||
| const isValidInputValue = (value?: string, inputType?: React.HTMLInputTypeAttribute) => { | ||
|
|
@@ -164,6 +164,7 @@ interface TextFieldBaseProps { | |
| value?: string; | ||
| inputRef?: React.Ref<HTMLInputElement | HTMLSelectElement>; | ||
| getSuggestions?: (value: string) => ReadonlyArray<string>; | ||
| shouldShowSuggestions?: 'focus' | number; | ||
| onClick?: (event: React.MouseEvent) => void; | ||
| onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void; | ||
| onBlur?: React.FocusEventHandler; | ||
|
|
@@ -475,8 +476,9 @@ export const TextFieldBase = React.forwardRef<any, TextFieldBaseProps>( | |
| const Autosuggest = React.lazy(() => import(/* webpackChunkName: "react-autosuggest" */ 'react-autosuggest')); | ||
|
|
||
| export const TextFieldBaseAutosuggest = React.forwardRef<any, TextFieldBaseProps>( | ||
| ({getSuggestions, id: idProp, ...props}, ref) => { | ||
| ({getSuggestions, id: idProp, shouldShowSuggestions = 'focus', ...props}, ref) => { | ||
| const [suggestions, setSuggestions] = React.useState<ReadonlyArray<string>>([]); | ||
| const [isEmptyCase, setIsEmptyCase] = React.useState(false); | ||
| const inputRef = React.useRef<HTMLInputElement>(null); | ||
| const containerRef = React.useRef<HTMLDivElement>(null); | ||
| const {platformOverrides, texts, t} = useTheme(); | ||
|
|
@@ -524,15 +526,26 @@ export const TextFieldBaseAutosuggest = React.forwardRef<any, TextFieldBaseProps | |
| return ( | ||
| <TextFieldBase | ||
| key={key} | ||
| role="combobox" // react-autosuggest adds this role to the container, but according to ARIA specs it should be on the input | ||
| {...(inputPropsWithoutKey as TextFieldBaseProps)} | ||
| fieldRef={containerRef} | ||
| inputRef={combineRefs(inputRef, props.inputRef, ref)} | ||
| /> | ||
| ); | ||
| }} | ||
| suggestions={suggestions} | ||
| onSuggestionsFetchRequested={({value}) => setSuggestions(getSuggestions(value))} | ||
| onSuggestionsClearRequested={() => setSuggestions([])} | ||
| shouldRenderSuggestions={(value) => | ||
| shouldShowSuggestions === 'focus' || value.trim().length >= shouldShowSuggestions | ||
| } | ||
| onSuggestionsFetchRequested={({value}) => { | ||
| const matchedSuggestions = getSuggestions(value); | ||
| setIsEmptyCase(matchedSuggestions.length === 0); | ||
| setSuggestions(matchedSuggestions); | ||
|
Comment on lines
+542
to
+543
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't this repeated state? you don't need an extra state for this. const isEmptyCase = suggestions.length === 0 |
||
| }} | ||
| onSuggestionsClearRequested={() => { | ||
| setIsEmptyCase(false); | ||
| setSuggestions([]); | ||
| }} | ||
| getSuggestionValue={(suggestion: any) => suggestion} | ||
| renderSuggestion={(suggestion: any, {isHighlighted}) => ( | ||
| <div | ||
|
|
@@ -545,9 +558,24 @@ export const TextFieldBaseAutosuggest = React.forwardRef<any, TextFieldBaseProps | |
| </div> | ||
| )} | ||
| renderSuggestionsContainer={(options) => { | ||
| if (!isEmptyCase && options.children === null) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to add this because of the padding. An empty container was rendered when there were no options |
||
| return null; | ||
| } | ||
|
|
||
| // extract key from containerProps to avoid React warning: | ||
| // "A props object containing a "key" prop is being spread into JSX" | ||
| const {key, ...containerPropsWithoutKey} = options.containerProps; | ||
| const children = isEmptyCase ? ( | ||
| <div role="status" className={classNames(styles.emptyCase)}> | ||
| <Text3 regular color={vars.colors.textSecondary}> | ||
| {texts.searchFieldSuggestionsEmptyCase || | ||
| t(tokens.searchFieldSuggestionsEmptyCase)} | ||
|
Comment on lines
+571
to
+572
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this should be configurable at field level |
||
| </Text3> | ||
| </div> | ||
| ) : ( | ||
| options.children | ||
| ); | ||
|
|
||
| return ( | ||
| <div | ||
| {...containerPropsWithoutKey} | ||
|
|
@@ -558,7 +586,7 @@ export const TextFieldBaseAutosuggest = React.forwardRef<any, TextFieldBaseProps | |
| className={styles.suggestionsContainer} | ||
| aria-label={`${props.label} ${texts.menuLabelSuffix || t(tokens.menuLabelSuffix)}`} | ||
| > | ||
| {options.children} | ||
| {children} | ||
| </div> | ||
| ); | ||
| }} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.