From b31b234d6501ac33a80ae8285f43a76eb9038176 Mon Sep 17 00:00:00 2001 From: dakahn Date: Tue, 3 Aug 2021 15:59:01 -0500 Subject: [PATCH 1/7] fix(Combobox): add event propagation to mouse down --- packages/react/src/components/ComboBox/ComboBox.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox.js b/packages/react/src/components/ComboBox/ComboBox.js index 325e1a439a98..da1117f977ea 100644 --- a/packages/react/src/components/ComboBox/ComboBox.js +++ b/packages/react/src/components/ComboBox/ComboBox.js @@ -255,8 +255,10 @@ const ComboBox = React.forwardRef((props, ref) => { // stop the event from propagating to prevent this. This allows the // toggleMenu behavior for the toggleButton to correctly open and // close the menu. - onMouseUp(event) { - event.stopPropagation(); + onMouseUp() { + if (textInput?.current) { + textInput.current.focus(); + } }, }); const inputProps = getInputProps({ From 606c61b1eec7855f352a81b28c1acf655c64182d Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Thu, 26 Aug 2021 11:25:10 -0500 Subject: [PATCH 2/7] fix(combobox): ignore downshift behavior for chevron click when listbox is open --- .../src/components/ComboBox/ComboBox-story.js | 35 +++++++++++++------ .../react/src/components/ComboBox/ComboBox.js | 10 +++--- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox-story.js b/packages/react/src/components/ComboBox/ComboBox-story.js index e82284bf4662..15da9623ad05 100644 --- a/packages/react/src/components/ComboBox/ComboBox-story.js +++ b/packages/react/src/components/ComboBox/ComboBox-story.js @@ -61,17 +61,30 @@ export default { }; export const combobox = () => ( -
- {}} - id="carbon-combobox" - items={items} - itemToString={(item) => (item ? item.text : '')} - placeholder="Filter..." - titleText="ComboBox title" - helperText="Combobox helper text" - /> -
+ <> +
+ {}} + id="carbon-combobox-1" + items={items} + itemToString={(item) => (item ? item.text : '')} + placeholder="Filter..." + titleText="ComboBox title" + helperText="Combobox helper text" + /> +
+
+ {}} + id="carbon-combobox-2" + items={items} + itemToString={(item) => (item ? item.text : '')} + placeholder="Filter..." + titleText="ComboBox title" + helperText="Combobox helper text" + /> +
+ ); const props = () => ({ diff --git a/packages/react/src/components/ComboBox/ComboBox.js b/packages/react/src/components/ComboBox/ComboBox.js index da1117f977ea..a41e9c03163d 100644 --- a/packages/react/src/components/ComboBox/ComboBox.js +++ b/packages/react/src/components/ComboBox/ComboBox.js @@ -252,12 +252,12 @@ const ComboBox = React.forwardRef((props, ref) => { // https://github.com/downshift-js/downshift/blob/v5.2.1/src/downshift.js#L1051-L1065 // // As a result, it will reset the state of the component and so we - // stop the event from propagating to prevent this. This allows the - // toggleMenu behavior for the toggleButton to correctly open and + // stop the event from propagating to prevent this if the menu is already open. + // This allows the toggleMenu behavior for the toggleButton to correctly open and // close the menu. - onMouseUp() { - if (textInput?.current) { - textInput.current.focus(); + onMouseUp(event) { + if (isOpen) { + event.stopPropagation(); } }, }); From 6f62e440ffe6ffc5b8927a4db94d8ce9abf70a7d Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Thu, 26 Aug 2021 11:36:04 -0500 Subject: [PATCH 3/7] docs(combobox): add multiple instances story --- .../src/components/ComboBox/ComboBox-story.js | 62 ++++++++++++------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox-story.js b/packages/react/src/components/ComboBox/ComboBox-story.js index 15da9623ad05..bed8c1e03794 100644 --- a/packages/react/src/components/ComboBox/ComboBox-story.js +++ b/packages/react/src/components/ComboBox/ComboBox-story.js @@ -61,30 +61,17 @@ export default { }; export const combobox = () => ( - <> -
- {}} - id="carbon-combobox-1" - items={items} - itemToString={(item) => (item ? item.text : '')} - placeholder="Filter..." - titleText="ComboBox title" - helperText="Combobox helper text" - /> -
-
- {}} - id="carbon-combobox-2" - items={items} - itemToString={(item) => (item ? item.text : '')} - placeholder="Filter..." - titleText="ComboBox title" - helperText="Combobox helper text" - /> -
- +
+ {}} + id="carbon-combobox-1" + items={items} + itemToString={(item) => (item ? item.text : '')} + placeholder="Filter..." + titleText="ComboBox title" + helperText="Combobox helper text" + /> +
); const props = () => ({ @@ -158,3 +145,30 @@ export const light = () => ( /> ); + +export const multipleInstances = () => ( + <> +
+ {}} + id="carbon-combobox-1" + items={items} + itemToString={(item) => (item ? item.text : '')} + placeholder="Filter..." + titleText="ComboBox title" + helperText="Combobox helper text" + /> +
+
+ {}} + id="carbon-combobox-2" + items={items} + itemToString={(item) => (item ? item.text : '')} + placeholder="Filter..." + titleText="ComboBox title" + helperText="Combobox helper text" + /> +
+ +); From 32ed27fedded70bb94b50f41c76f174cb0535ee9 Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Thu, 26 Aug 2021 14:33:26 -0500 Subject: [PATCH 4/7] test(combobox): multiple instances should only render one listbox at a time --- .../src/components/ComboBox/ComboBox-test.js | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/react/src/components/ComboBox/ComboBox-test.js b/packages/react/src/components/ComboBox/ComboBox-test.js index 2252549a8cb8..c068ddbc911e 100644 --- a/packages/react/src/components/ComboBox/ComboBox-test.js +++ b/packages/react/src/components/ComboBox/ComboBox-test.js @@ -7,6 +7,8 @@ import React from 'react'; import { mount } from 'enzyme'; +import { render, screen, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { findListBoxNode, findMenuNode, @@ -201,5 +203,45 @@ describe('ComboBox', () => { const wrapper = mount(); expect(wrapper.find('input').instance().value).toBe(''); }); + + it('should only render one listbox at a time when multiple comboboxes are present', () => { + render( + <> +
+ +
+
+ +
+ + ); + const firstCombobox = screen.getByTestId('combobox-1'); + const secondCombobox = screen.getByTestId('combobox-2'); + + const firstComboboxChevron = within(firstCombobox).getByRole('button'); + const secondComboboxChevron = within(secondCombobox).getByRole('button'); + + expect(within(firstCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + expect(within(secondCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + + userEvent.click(firstComboboxChevron); + + expect( + within(firstCombobox).getByRole('listbox') + ).not.toBeEmptyDOMElement(); + expect(within(secondCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + + userEvent.click(secondComboboxChevron); + + expect(within(firstCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + expect( + within(secondCombobox).getByRole('listbox') + ).not.toBeEmptyDOMElement(); + + userEvent.click(secondComboboxChevron); + + expect(within(firstCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + expect(within(secondCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + }); }); }); From 54af89a22d2a5e741b74b475f8d3962078cc75c6 Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Thu, 26 Aug 2021 14:34:04 -0500 Subject: [PATCH 5/7] chore: remove unecessary id value --- packages/react/src/components/ComboBox/ComboBox-story.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/components/ComboBox/ComboBox-story.js b/packages/react/src/components/ComboBox/ComboBox-story.js index bed8c1e03794..8788d0655338 100644 --- a/packages/react/src/components/ComboBox/ComboBox-story.js +++ b/packages/react/src/components/ComboBox/ComboBox-story.js @@ -64,7 +64,7 @@ export const combobox = () => (
{}} - id="carbon-combobox-1" + id="carbon-combobox" items={items} itemToString={(item) => (item ? item.text : '')} placeholder="Filter..." From 9f7d2a9d182233f0cea02d4f9e224df5cf7e2863 Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Thu, 26 Aug 2021 14:38:23 -0500 Subject: [PATCH 6/7] test(combobox): add query abstraction for readability --- .../src/components/ComboBox/ComboBox-test.js | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox-test.js b/packages/react/src/components/ComboBox/ComboBox-test.js index c068ddbc911e..7bfe47f8f4b2 100644 --- a/packages/react/src/components/ComboBox/ComboBox-test.js +++ b/packages/react/src/components/ComboBox/ComboBox-test.js @@ -221,27 +221,31 @@ describe('ComboBox', () => { const firstComboboxChevron = within(firstCombobox).getByRole('button'); const secondComboboxChevron = within(secondCombobox).getByRole('button'); - expect(within(firstCombobox).getByRole('listbox')).toBeEmptyDOMElement(); - expect(within(secondCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + function firstListBox() { + return within(firstCombobox).getByRole('listbox'); + } + + function secondListBox() { + return within(secondCombobox).getByRole('listbox'); + } + + expect(firstListBox()).toBeEmptyDOMElement(); + expect(secondListBox()).toBeEmptyDOMElement(); userEvent.click(firstComboboxChevron); - expect( - within(firstCombobox).getByRole('listbox') - ).not.toBeEmptyDOMElement(); - expect(within(secondCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + expect(firstListBox()).not.toBeEmptyDOMElement(); + expect(secondListBox()).toBeEmptyDOMElement(); userEvent.click(secondComboboxChevron); - expect(within(firstCombobox).getByRole('listbox')).toBeEmptyDOMElement(); - expect( - within(secondCombobox).getByRole('listbox') - ).not.toBeEmptyDOMElement(); + expect(firstListBox()).toBeEmptyDOMElement(); + expect(secondListBox()).not.toBeEmptyDOMElement(); userEvent.click(secondComboboxChevron); - expect(within(firstCombobox).getByRole('listbox')).toBeEmptyDOMElement(); - expect(within(secondCombobox).getByRole('listbox')).toBeEmptyDOMElement(); + expect(firstListBox()).toBeEmptyDOMElement(); + expect(secondListBox()).toBeEmptyDOMElement(); }); }); }); From 516cd2461925a6cf9413ec0eaf1b563137c9a325 Mon Sep 17 00:00:00 2001 From: TJ Egan Date: Thu, 2 Sep 2021 13:28:32 +0200 Subject: [PATCH 7/7] fix(Combobox): remove test story --- .../src/components/ComboBox/ComboBox-story.js | 27 ------------------- 1 file changed, 27 deletions(-) diff --git a/packages/react/src/components/ComboBox/ComboBox-story.js b/packages/react/src/components/ComboBox/ComboBox-story.js index 8788d0655338..e82284bf4662 100644 --- a/packages/react/src/components/ComboBox/ComboBox-story.js +++ b/packages/react/src/components/ComboBox/ComboBox-story.js @@ -145,30 +145,3 @@ export const light = () => ( />
); - -export const multipleInstances = () => ( - <> -
- {}} - id="carbon-combobox-1" - items={items} - itemToString={(item) => (item ? item.text : '')} - placeholder="Filter..." - titleText="ComboBox title" - helperText="Combobox helper text" - /> -
-
- {}} - id="carbon-combobox-2" - items={items} - itemToString={(item) => (item ? item.text : '')} - placeholder="Filter..." - titleText="ComboBox title" - helperText="Combobox helper text" - /> -
- -);