diff --git a/apps/vr-tests-react-components/src/stories/Combobox.stories.tsx b/apps/vr-tests-react-components/src/stories/Combobox.stories.tsx index 1734d1b7a737ad..0b237f2954af68 100644 --- a/apps/vr-tests-react-components/src/stories/Combobox.stories.tsx +++ b/apps/vr-tests-react-components/src/stories/Combobox.stories.tsx @@ -50,7 +50,7 @@ storiesOf('Combobox Converged', module) )) .addStory('Disabled with value', () => ( - + )) @@ -88,13 +88,6 @@ storiesOf('Combobox Converged', module) )) - .addStory('With multiselect value', () => ( - - - - - - )) .addStory('Size: small', () => ( @@ -152,6 +145,24 @@ storiesOf('Combobox Converged', module) )) + .addStory('With selection', () => ( +
+ + + + + +
+ )) + .addStory('With multiselect selection', () => ( +
+ + + + + +
+ )) .addStory('Disabled option', () => (
diff --git a/apps/vr-tests-react-components/src/stories/Dropdown.stories.tsx b/apps/vr-tests-react-components/src/stories/Dropdown.stories.tsx index a1264786512fe5..9140191c9fd338 100644 --- a/apps/vr-tests-react-components/src/stories/Dropdown.stories.tsx +++ b/apps/vr-tests-react-components/src/stories/Dropdown.stories.tsx @@ -50,7 +50,7 @@ storiesOf('Dropdown Converged', module) )) .addStory('Disabled with value', () => ( - + )) @@ -91,7 +91,7 @@ storiesOf('Dropdown Converged', module) )) .addStory('With multiselect value', () => ( - + @@ -148,6 +148,20 @@ storiesOf('Dropdown Converged', module) )) + .addStory('With selection', () => ( + + + + + + )) + .addStory('With multiselect selection', () => ( + + + + + + )) .addStory('Disabled option', () => ( diff --git a/change/@fluentui-react-combobox-6312fcfc-fc81-4a08-9c2e-b3a654ee49a0.json b/change/@fluentui-react-combobox-6312fcfc-fc81-4a08-9c2e-b3a654ee49a0.json new file mode 100644 index 00000000000000..59419d9320c443 --- /dev/null +++ b/change/@fluentui-react-combobox-6312fcfc-fc81-4a08-9c2e-b3a654ee49a0.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "fix: update Combobox and Dropdown to never use the Option value string as a display value; input value must be set if selectedOptions or defaultSelectOptions is set\"", + "packageName": "@fluentui/react-combobox", + "email": "sarah.higley@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-combobox/src/components/Combobox/Combobox.test.tsx b/packages/react-components/react-combobox/src/components/Combobox/Combobox.test.tsx index 928b1c1310ce05..0eece7c10057d6 100644 --- a/packages/react-components/react-combobox/src/components/Combobox/Combobox.test.tsx +++ b/packages/react-components/react-combobox/src/components/Combobox/Combobox.test.tsx @@ -407,6 +407,20 @@ describe('Combobox', () => { expect(getByTestId('green').getAttribute('aria-checked')).toEqual('false'); }); + it('should set display value to option text', () => { + const { getByRole, getByText } = render( + + + + + , + ); + + userEvent.click(getByText('Green')); + + expect((getByRole('combobox') as HTMLInputElement).value).toEqual('Green'); + }); + it('should change defaultSelectedOptions on click', () => { const { getByTestId } = render( diff --git a/packages/react-components/react-combobox/src/components/Combobox/useCombobox.tsx b/packages/react-components/react-combobox/src/components/Combobox/useCombobox.tsx index a57e3796c19e7c..a9c6761cce3d24 100644 --- a/packages/react-components/react-combobox/src/components/Combobox/useCombobox.tsx +++ b/packages/react-components/react-combobox/src/components/Combobox/useCombobox.tsx @@ -107,7 +107,7 @@ export const useCombobox_unstable = (props: ComboboxProps, ref: React.Ref { }); }); + it('should set display value to option text', () => { + const { getByRole, getByText } = render( + + + + + , + ); + + fireEvent.click(getByText('Green')); + + expect(getByRole('combobox').textContent).toEqual('Green'); + }); + it('calls onOptionSelect with correct data for multi-select', () => { const onOptionSelect = jest.fn(); diff --git a/packages/react-components/react-combobox/src/components/Dropdown/useDropdown.tsx b/packages/react-components/react-combobox/src/components/Dropdown/useDropdown.tsx index 4f1c8a3cf7e694..ef1b750b9da4a0 100644 --- a/packages/react-components/react-combobox/src/components/Dropdown/useDropdown.tsx +++ b/packages/react-components/react-combobox/src/components/Dropdown/useDropdown.tsx @@ -52,7 +52,7 @@ export const useDropdown_unstable = (props: DropdownProps, ref: React.Ref { // first check for matches for the full searchString - let matcher = (optionValue: string) => optionValue.toLowerCase().indexOf(searchString.current) === 0; + let matcher = (optionText: string) => optionText.toLowerCase().indexOf(searchString.current) === 0; let matches = getOptionsMatchingText(matcher); let startIndex = activeOption ? getIndexOfId(activeOption.id) : 0; @@ -71,7 +71,7 @@ export const useDropdown_unstable = (props: DropdownProps, ref: React.Ref optionValue.toLowerCase().indexOf(letters[0]) === 0; + matcher = (optionText: string) => optionText.toLowerCase().indexOf(letters[0]) === 0; matches = getOptionsMatchingText(matcher); } } diff --git a/packages/react-components/react-combobox/src/utils/OptionCollection.types.ts b/packages/react-components/react-combobox/src/utils/OptionCollection.types.ts index 47e2197ef890fc..795194220466e5 100644 --- a/packages/react-components/react-combobox/src/utils/OptionCollection.types.ts +++ b/packages/react-components/react-combobox/src/utils/OptionCollection.types.ts @@ -26,7 +26,10 @@ export type OptionCollectionState = { getOptionById(id: string): OptionValue | undefined; /** Returns an array of options filtered by a value matching function against the option's text string. */ - getOptionsMatchingText(matcher: (value: string) => boolean): OptionValue[]; + getOptionsMatchingText(matcher: (text: string) => boolean): OptionValue[]; + + /** Returns an array of options filtered by a value matching function against the option's value string. */ + getOptionsMatchingValue(matcher: (value: string) => boolean): OptionValue[]; /** The unordered option data. */ options: OptionValue[]; diff --git a/packages/react-components/react-combobox/src/utils/Selection.types.ts b/packages/react-components/react-combobox/src/utils/Selection.types.ts index 1617a13e0db852..6e8c6150c0094a 100644 --- a/packages/react-components/react-combobox/src/utils/Selection.types.ts +++ b/packages/react-components/react-combobox/src/utils/Selection.types.ts @@ -2,7 +2,10 @@ import * as React from 'react'; import { OptionValue } from './OptionCollection.types'; export type SelectionProps = { - /* For an uncontrolled component, sets the initial selection */ + /** + * For an uncontrolled component, sets the initial selection. + * If this is set, the `defaultValue` prop MUST also be set. + */ defaultSelectedOptions?: string[]; /** @@ -19,6 +22,7 @@ export type SelectionProps = { /** * An array of selected option keys. * Use this with `onOptionSelect` to directly control the selected option(s) + * If this is set, the `value` prop MUST also be controlled. */ selectedOptions?: string[]; }; diff --git a/packages/react-components/react-combobox/src/utils/useComboboxBaseState.ts b/packages/react-components/react-combobox/src/utils/useComboboxBaseState.ts index 005255b1e59b9a..4646284e05765b 100644 --- a/packages/react-components/react-combobox/src/utils/useComboboxBaseState.ts +++ b/packages/react-components/react-combobox/src/utils/useComboboxBaseState.ts @@ -22,7 +22,7 @@ export const useComboboxBaseState = ( } = props; const optionCollection = useOptionCollection(); - const { getOptionAtIndex, getOptionsMatchingText } = optionCollection; + const { getOptionAtIndex, getOptionsMatchingValue } = optionCollection; const [activeOption, setActiveOption] = React.useState(); @@ -56,13 +56,22 @@ export const useComboboxBaseState = ( return props.defaultValue; } + const selectedOptionsText = getOptionsMatchingValue(optionValue => { + return selectedOptions.includes(optionValue); + }).map(option => option.text); + if (multiselect) { // editable inputs should not display multiple selected options in the input as text - return editable ? '' : selectedOptions.join(', '); + return editable ? '' : selectedOptionsText.join(', '); } - return selectedOptions[0]; - }, [controllableValue, editable, isFirstMount, multiselect, props.defaultValue, selectedOptions]); + return selectedOptionsText[0]; + + // do not change value after isFirstMount changes, + // we do not want to accidentally override defaultValue on a second render + // unless another value is intentionally set + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [controllableValue, editable, getOptionsMatchingValue, multiselect, props.defaultValue, selectedOptions]); // Handle open state, which is shared with options in context const [open, setOpenState] = useControllableState({ @@ -84,7 +93,7 @@ export const useComboboxBaseState = ( if (open && !activeOption) { // if it is single-select and there is a selected option, start at the selected option if (!multiselect && selectedOptions.length > 0) { - const selectedOption = getOptionsMatchingText(v => v === selectedOptions[0]).pop(); + const selectedOption = getOptionsMatchingValue(v => v === selectedOptions[0]).pop(); selectedOption && setActiveOption(selectedOption); } // default to starting at the first option diff --git a/packages/react-components/react-combobox/src/utils/useOptionCollection.ts b/packages/react-components/react-combobox/src/utils/useOptionCollection.ts index 3df8d206827ef4..9bff10806e5254 100644 --- a/packages/react-components/react-combobox/src/utils/useOptionCollection.ts +++ b/packages/react-components/react-combobox/src/utils/useOptionCollection.ts @@ -15,9 +15,12 @@ export const useOptionCollection = (): OptionCollectionState => { const item = nodes.current.find(node => node.option.id === id); return item?.option; }; - const getOptionsMatchingText = (matcher: (value: string) => boolean) => { + const getOptionsMatchingText = (matcher: (text: string) => boolean) => { return nodes.current.filter(node => matcher(node.option.text)).map(node => node.option); }; + const getOptionsMatchingValue = (matcher: (value: string) => boolean) => { + return nodes.current.filter(node => matcher(node.option.value)).map(node => node.option); + }; return { getCount, @@ -25,6 +28,7 @@ export const useOptionCollection = (): OptionCollectionState => { getIndexOfId, getOptionById, getOptionsMatchingText, + getOptionsMatchingValue, }; }, []); diff --git a/packages/react-components/react-combobox/stories/Combobox/ComboboxControlled.stories.tsx b/packages/react-components/react-combobox/stories/Combobox/ComboboxControlled.stories.tsx new file mode 100644 index 00000000000000..da3a0dfa5f8a61 --- /dev/null +++ b/packages/react-components/react-combobox/stories/Combobox/ComboboxControlled.stories.tsx @@ -0,0 +1,149 @@ +import * as React from 'react'; +import { Combobox, makeStyles, Option, shorthands, useId, Persona } from '@fluentui/react-components'; +import type { ComboboxProps } from '@fluentui/react-components'; + +const useStyles = makeStyles({ + root: { + // Stack the label above the field with a gap + display: 'grid', + justifyItems: 'start', + ...shorthands.gap('20px'), + maxWidth: '400px', + }, + field: { + display: 'grid', + justifyItems: 'start', + ...shorthands.gap('2px'), + }, +}); + +export const Controlled = (props: Partial) => { + const comboId = useId('combo-controlled'); + const styles = useStyles(); + const [selectedOptions, setSelectedOptions] = React.useState(['eatkins']); + const [value, setValue] = React.useState('Elvia Atkins'); + + const onOptionSelect: typeof props['onOptionSelect'] = (ev, data) => { + setSelectedOptions(data.selectedOptions); + setValue(data.optionText ?? ''); + }; + + const onInput = (ev: React.ChangeEvent) => { + setValue(ev.target.value); + }; + + return ( +
+
+ + + + + + + +
+ +
+ + + + + + + +
+
+ ); +}; + +Controlled.parameters = { + docs: { + description: { + story: + 'A Combobox may have controlled or controlled selection and value. ' + + 'When the selection is controlled or a default selection is provided, ' + + 'a controlled value or default value must also be defined. ' + + 'Otherwise, the Combobox will not be able to display a value before the Options are rendered.', + }, + }, +}; diff --git a/packages/react-components/react-combobox/stories/Combobox/index.stories.tsx b/packages/react-components/react-combobox/stories/Combobox/index.stories.tsx index 420c1a760fbda6..a25b1e71eb9a85 100644 --- a/packages/react-components/react-combobox/stories/Combobox/index.stories.tsx +++ b/packages/react-components/react-combobox/stories/Combobox/index.stories.tsx @@ -7,6 +7,7 @@ import bestPracticesMd from './ComboboxBestPractices.md'; export { Default } from './ComboboxDefault.stories'; export { ComplexOptions } from './ComboboxComplexOptions.stories'; export { CustomOptions } from './ComboboxCustomOptions.stories'; +export { Controlled } from './ComboboxControlled.stories'; export { Filtering } from './ComboboxFiltering.stories'; export { Freeform } from './ComboboxFreeform.stories'; export { Multiselect } from './ComboboxMultiselect.stories'; diff --git a/packages/react-components/react-combobox/stories/Dropdown/DropdownControlled.stories.tsx b/packages/react-components/react-combobox/stories/Dropdown/DropdownControlled.stories.tsx new file mode 100644 index 00000000000000..5475e8c653578c --- /dev/null +++ b/packages/react-components/react-combobox/stories/Dropdown/DropdownControlled.stories.tsx @@ -0,0 +1,144 @@ +import * as React from 'react'; +import { Dropdown, makeStyles, Option, shorthands, useId, Persona } from '@fluentui/react-components'; +import type { DropdownProps } from '@fluentui/react-components'; + +const useStyles = makeStyles({ + root: { + // Stack the label above the field with a gap + display: 'grid', + justifyItems: 'start', + ...shorthands.gap('20px'), + maxWidth: '400px', + }, + field: { + display: 'grid', + justifyItems: 'start', + ...shorthands.gap('2px'), + }, +}); + +export const Controlled = (props: Partial) => { + const comboId = useId('combo-controlled'); + const styles = useStyles(); + const [selectedOptions, setSelectedOptions] = React.useState(['eatkins']); + const [value, setValue] = React.useState('Elvia Atkins'); + + const onOptionSelect: typeof props['onOptionSelect'] = (ev, data) => { + setSelectedOptions(data.selectedOptions); + setValue(data.optionText ?? ''); + }; + + return ( +
+
+ + + + + + + +
+ +
+ + + + + + + +
+
+ ); +}; + +Controlled.parameters = { + docs: { + description: { + story: + 'A Dropdown may have controlled or controlled selection and value. ' + + 'When the selection is controlled or a default selection is provided, ' + + 'a controlled value or default value must also be defined. ' + + 'Otherwise, the Dropdown will not be able to display a value before the Options are rendered.', + }, + }, +}; diff --git a/packages/react-components/react-combobox/stories/Dropdown/index.stories.tsx b/packages/react-components/react-combobox/stories/Dropdown/index.stories.tsx index 8d65c70459845a..546a5d51915861 100644 --- a/packages/react-components/react-combobox/stories/Dropdown/index.stories.tsx +++ b/packages/react-components/react-combobox/stories/Dropdown/index.stories.tsx @@ -10,6 +10,7 @@ export { Appearance } from './DropdownAppearance.stories'; export { Grouped } from './DropdownGrouped.stories'; export { ComplexOptions } from './DropdownComplexOptions.stories'; export { CustomOptions } from './DropdownCustomOptions.stories'; +export { Controlled } from './DropdownControlled.stories'; export { Multiselect } from './DropdownMultiselect.stories'; export { Size } from './DropdownSize.stories'; export { Disabled } from './DropdownDisabled.stories';