feat: change Combobox/Dropdown Option to require label and use `val…

…ue` for non-display text (#25379)
microsoft · Dec 3, 2022 · 84a26d8 · 84a26d8
1 parent 7f4466d
commit 84a26d8
Show file tree

Hide file tree

Showing 21 changed files with 428 additions and 44 deletions.
diff --git a/change/@fluentui-react-combobox-11a96c34-a691-46b7-800f-207349570f7b.json b/change/@fluentui-react-combobox-11a96c34-a691-46b7-800f-207349570f7b.json
@@ -0,0 +1,7 @@
+{
+  "type": "prerelease",
+  "comment": " feat: update Option value to be a non-display value, and add an Option text prop that is required if the child is not a string",
+  "packageName": "@fluentui/react-combobox",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/react-components/react-combobox/etc/react-combobox.api.md b/packages/react-components/react-combobox/etc/react-combobox.api.md
@@ -160,7 +160,13 @@ export type OptionGroupState = ComponentState<OptionGroupSlots>;
 export type OptionProps = ComponentProps<Partial<OptionSlots>> & {
     disabled?: boolean;
     value?: string;
-};
+} & ({
+    text?: string;
+    children: string;
+} | {
+    text: string;
+    children?: React_2.ReactNode;
+});
 
 // @public (undocumented)
 export type OptionSlots = {

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
@@ -278,6 +278,23 @@ describe('Combobox', () => {
     expect(getByTestId('blue').getAttribute('aria-selected')).toEqual('false');
   });
 
+  it('should set defaultSelectedOptions based on Option `value`', () => {
+    const { getByTestId } = render(
+      <Combobox open multiselect defaultSelectedOptions={['b', 'c']}>
+        <Option value="a">Red</Option>
+        <Option data-testid="green" value="b">
+          Green
+        </Option>
+        <Option data-testid="blue" value="c">
+          Blue
+        </Option>
+      </Combobox>,
+    );
+
+    expect(getByTestId('green').getAttribute('aria-selected')).toEqual('true');
+    expect(getByTestId('blue').getAttribute('aria-selected')).toEqual('true');
+  });
+
   it('should set selectedOptions', () => {
     const { getByTestId } = render(
       <Combobox open selectedOptions={['Green']}>
@@ -290,6 +307,26 @@ describe('Combobox', () => {
     expect(getByTestId('green').getAttribute('aria-selected')).toEqual('true');
   });
 
+  it('should set selectedOptions based on Option `value`', () => {
+    const { getByTestId } = render(
+      <Combobox open multiselect selectedOptions={['a', 'c']}>
+        <Option data-testid="red" value="a">
+          Red
+        </Option>
+        <Option data-testid="green" value="b">
+          Green
+        </Option>
+        <Option data-testid="blue" value="c">
+          Blue
+        </Option>
+      </Combobox>,
+    );
+
+    expect(getByTestId('red').getAttribute('aria-selected')).toEqual('true');
+    expect(getByTestId('blue').getAttribute('aria-selected')).toEqual('true');
+    expect(getByTestId('green').getAttribute('aria-selected')).toEqual('false');
+  });
+
   it('should change defaultSelectedOptions on click', () => {
     const { getByTestId } = render(
       <Combobox open defaultSelectedOptions={['Green']}>
@@ -458,10 +495,33 @@ describe('Combobox', () => {
     expect(onOptionSelect).toHaveBeenCalledTimes(1);
     expect(onOptionSelect).toHaveBeenCalledWith(expect.anything(), {
       optionValue: 'Green',
+      optionText: 'Green',
       selectedOptions: ['Green'],
     });
   });
 
+  it('calls onOptionSelect with Option value prop', () => {
+    const onOptionSelect = jest.fn();
+
+    const { getByRole, getByText } = render(
+      <Combobox value="Red" onOptionSelect={onOptionSelect}>
+        <Option>Red</Option>
+        <Option value="test">Green</Option>
+        <Option>Blue</Option>
+      </Combobox>,
+    );
+
+    userEvent.click(getByRole('combobox'));
+    userEvent.click(getByText('Green'));
+
+    expect(onOptionSelect).toHaveBeenCalledTimes(1);
+    expect(onOptionSelect).toHaveBeenCalledWith(expect.anything(), {
+      optionValue: 'test',
+      optionText: 'Green',
+      selectedOptions: ['test'],
+    });
+  });
+
   it('calls onOptionSelect with correct data for multi-select', () => {
     const onOptionSelect = jest.fn();
 
@@ -480,10 +540,12 @@ describe('Combobox', () => {
     expect(onOptionSelect).toHaveBeenCalledTimes(2);
     expect(onOptionSelect).toHaveBeenNthCalledWith(1, expect.anything(), {
       optionValue: 'Green',
+      optionText: 'Green',
       selectedOptions: ['Green'],
     });
     expect(onOptionSelect).toHaveBeenNthCalledWith(2, expect.anything(), {
       optionValue: 'Blue',
+      optionText: 'Blue',
       selectedOptions: ['Green', 'Blue'],
     });
   });

diff --git a/packages/react-components/react-combobox/src/components/Combobox/useCombobox.tsx b/packages/react-components/react-combobox/src/components/Combobox/useCombobox.tsx
@@ -32,7 +32,7 @@ export const useCombobox_unstable = (props: ComboboxProps, ref: React.Ref<HTMLIn
     activeOption,
     clearSelection,
     getIndexOfId,
-    getOptionsMatchingValue,
+    getOptionsMatchingText,
     hasFocus,
     open,
     selectOption,
@@ -77,8 +77,8 @@ export const useCombobox_unstable = (props: ComboboxProps, ref: React.Ref<HTMLIn
       return;
     }
 
-    const matcher = (optionValue: string) => optionValue.toLowerCase().indexOf(searchString) === 0;
-    const matches = getOptionsMatchingValue(matcher);
+    const matcher = (optionText: string) => optionText.toLowerCase().indexOf(searchString) === 0;
+    const matches = getOptionsMatchingText(matcher);
 
     // return first matching option after the current active option, looping back to the top
     if (matches.length > 1 && activeOption) {

diff --git a/packages/react-components/react-combobox/src/components/Dropdown/Dropdown.test.tsx b/packages/react-components/react-combobox/src/components/Dropdown/Dropdown.test.tsx
@@ -214,6 +214,23 @@ describe('Dropdown', () => {
     expect(getByTestId('blue').getAttribute('aria-selected')).toEqual('false');
   });
 
+  it('should set defaultSelectedOptions based on Option `value`', () => {
+    const { getByTestId } = render(
+      <Dropdown open multiselect defaultSelectedOptions={['b', 'c']}>
+        <Option value="a">Red</Option>
+        <Option data-testid="green" value="b">
+          Green
+        </Option>
+        <Option data-testid="blue" value="c">
+          Blue
+        </Option>
+      </Dropdown>,
+    );
+
+    expect(getByTestId('green').getAttribute('aria-selected')).toEqual('true');
+    expect(getByTestId('blue').getAttribute('aria-selected')).toEqual('true');
+  });
+
   it('should set selectedOptions', () => {
     const { getByTestId } = render(
       <Dropdown open selectedOptions={['Green']}>
@@ -226,6 +243,26 @@ describe('Dropdown', () => {
     expect(getByTestId('green').getAttribute('aria-selected')).toEqual('true');
   });
 
+  it('should set selectedOptions based on Option `value`', () => {
+    const { getByTestId } = render(
+      <Dropdown open multiselect selectedOptions={['a', 'c']}>
+        <Option data-testid="red" value="a">
+          Red
+        </Option>
+        <Option data-testid="green" value="b">
+          Green
+        </Option>
+        <Option data-testid="blue" value="c">
+          Blue
+        </Option>
+      </Dropdown>,
+    );
+
+    expect(getByTestId('red').getAttribute('aria-selected')).toEqual('true');
+    expect(getByTestId('blue').getAttribute('aria-selected')).toEqual('true');
+    expect(getByTestId('green').getAttribute('aria-selected')).toEqual('false');
+  });
+
   it('should change defaultSelectedOptions on click', () => {
     const { getByTestId } = render(
       <Dropdown open defaultSelectedOptions={['Green']}>
@@ -340,10 +377,33 @@ describe('Dropdown', () => {
     expect(onOptionSelect).toHaveBeenCalledTimes(1);
     expect(onOptionSelect).toHaveBeenCalledWith(expect.anything(), {
       optionValue: 'Green',
+      optionText: 'Green',
       selectedOptions: ['Green'],
     });
   });
 
+  it('calls onOptionSelect with Option value prop', () => {
+    const onOptionSelect = jest.fn();
+
+    const { getByRole, getByText } = render(
+      <Dropdown value="Red" onOptionSelect={onOptionSelect}>
+        <Option>Red</Option>
+        <Option value="test">Green</Option>
+        <Option>Blue</Option>
+      </Dropdown>,
+    );
+
+    userEvent.click(getByRole('combobox'));
+    userEvent.click(getByText('Green'));
+
+    expect(onOptionSelect).toHaveBeenCalledTimes(1);
+    expect(onOptionSelect).toHaveBeenCalledWith(expect.anything(), {
+      optionValue: 'test',
+      optionText: 'Green',
+      selectedOptions: ['test'],
+    });
+  });
+
   it('calls onOptionSelect with correct data for multi-select', () => {
     const onOptionSelect = jest.fn();
 
@@ -362,10 +422,12 @@ describe('Dropdown', () => {
     expect(onOptionSelect).toHaveBeenCalledTimes(2);
     expect(onOptionSelect).toHaveBeenNthCalledWith(1, expect.anything(), {
       optionValue: 'Green',
+      optionText: 'Green',
       selectedOptions: ['Green'],
     });
     expect(onOptionSelect).toHaveBeenNthCalledWith(2, expect.anything(), {
       optionValue: 'Blue',
+      optionText: 'Blue',
       selectedOptions: ['Green', 'Blue'],
     });
   });

diff --git a/packages/react-components/react-combobox/src/components/Dropdown/useDropdown.tsx b/packages/react-components/react-combobox/src/components/Dropdown/useDropdown.tsx
@@ -25,7 +25,7 @@ export const useDropdown_unstable = (props: DropdownProps, ref: React.Ref<HTMLBu
   const {
     activeOption,
     getIndexOfId,
-    getOptionsMatchingValue,
+    getOptionsMatchingText,
     open,
     setActiveOption,
     setFocusVisible,
@@ -53,7 +53,7 @@ export const useDropdown_unstable = (props: DropdownProps, ref: React.Ref<HTMLBu
   const getNextMatchingOption = (): OptionValue | undefined => {
     // first check for matches for the full searchString
     let matcher = (optionValue: string) => optionValue.toLowerCase().indexOf(searchString.current) === 0;
-    let matches = getOptionsMatchingValue(matcher);
+    let matches = getOptionsMatchingText(matcher);
     let startIndex = activeOption ? getIndexOfId(activeOption.id) : 0;
 
     // if the dropdown is already open and the searchstring is a single character,
@@ -72,7 +72,7 @@ export const useDropdown_unstable = (props: DropdownProps, ref: React.Ref<HTMLBu
       if (allSameLetter) {
         startIndex++;
         matcher = (optionValue: string) => optionValue.toLowerCase().indexOf(letters[0]) === 0;
-        matches = getOptionsMatchingValue(matcher);
+        matches = getOptionsMatchingText(matcher);
       }
     }
 

diff --git a/packages/react-components/react-combobox/src/components/Listbox/Listbox.test.tsx b/packages/react-components/react-combobox/src/components/Listbox/Listbox.test.tsx
@@ -219,7 +219,11 @@ describe('Listbox', () => {
     fireEvent.click(getByText('Red'));
 
     expect(onOptionSelect).toHaveBeenCalled();
-    expect(onOptionSelect.mock.calls[0][1]).toEqual({ optionValue: 'Red', selectedOptions: ['Red'] });
+    expect(onOptionSelect.mock.calls[0][1]).toEqual({
+      optionValue: 'Red',
+      optionText: 'Red',
+      selectedOptions: ['Red'],
+    });
   });
 
   it('should not change selection with selectedOptions', () => {
@@ -239,7 +243,11 @@ describe('Listbox', () => {
 
     expect(option.getAttribute('aria-selected')).toEqual('false');
     expect(onOptionSelect).toHaveBeenCalled();
-    expect(onOptionSelect.mock.calls[0][1]).toEqual({ optionValue: 'Red', selectedOptions: ['Red'] });
+    expect(onOptionSelect.mock.calls[0][1]).toEqual({
+      optionValue: 'Red',
+      optionText: 'Red',
+      selectedOptions: ['Red'],
+    });
   });
 
   it('should select option with the enter key', () => {