From 5d90112698887e182ba3042602c5d909c607ea13 Mon Sep 17 00:00:00 2001 From: Alan Greene Date: Thu, 7 Nov 2024 19:57:26 +0000 Subject: [PATCH] Adopt ComboBox selectedItem prop Remove previous workaround and adopt the `selectedItem` prop to make the `ComboBox` component fully controlled. To avoid the previous issue where the ComboBox would get into a loop or cause the tests to crash due to OOM, we memoise the value passed to `selectedItem` to ensure the same reference is kept if the value is unchanged, e.g. in case of a selected namespace: `{ id: namespace, text: namespace }` we memoise on the `id` field so the same object is returned unless the namespace changes. This avoid the ComboBox unnecessarily re-rendering, and also handles the case of translated text for the 'All namespaces' option. --- .../TooltipDropdown/TooltipDropdown.jsx | 3 +-- .../HeaderBarContent/HeaderBarContent.test.jsx | 12 ++++++------ .../NamespacesDropdown/NamespacesDropdown.jsx | 13 +++++++++---- .../NamespacesDropdown.test.jsx | 16 ++++++++++++---- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/packages/components/src/components/TooltipDropdown/TooltipDropdown.jsx b/packages/components/src/components/TooltipDropdown/TooltipDropdown.jsx index dffd4109d..190b9a492 100644 --- a/packages/components/src/components/TooltipDropdown/TooltipDropdown.jsx +++ b/packages/components/src/components/TooltipDropdown/TooltipDropdown.jsx @@ -74,12 +74,11 @@ const TooltipDropdown = ({ className={className} disabled={disabled} id={id} - initialSelectedItem={selectedItem} items={options} itemToString={itemToString} - key={JSON.stringify(selectedItem)} onChange={onChange} placeholder={options.length === 0 ? emptyString : label} + selectedItem={selectedItem} size={size} titleText={titleText} translateWithId={getTranslateWithId(intl)} diff --git a/src/containers/HeaderBarContent/HeaderBarContent.test.jsx b/src/containers/HeaderBarContent/HeaderBarContent.test.jsx index 42b51e8c9..1ae36e8f3 100644 --- a/src/containers/HeaderBarContent/HeaderBarContent.test.jsx +++ b/src/containers/HeaderBarContent/HeaderBarContent.test.jsx @@ -30,7 +30,7 @@ describe('HeaderBarContent', () => { const path = '/namespaces/:namespace/foo'; renderWithRouter(, { path, - route: `/namespaces/${namespace}/foo` + route: path.replace(':namespace', namespace) }); expect(selectNamespace).toHaveBeenCalledWith(namespace); }); @@ -76,7 +76,7 @@ describe('HeaderBarContent', () => { { handle: { isNamespaced: true, path }, path, - route: `/namespaces/${namespace}/fake/path` + route: path.replace(':namespace', namespace) } ); fireEvent.click(getByDisplayValue(namespace)); @@ -103,7 +103,7 @@ describe('HeaderBarContent', () => { { handle: { isNamespaced: true, path }, path, - route: `/namespaces/${namespace}/fake/path` + route: path.replace(':namespace', namespace) } ); fireEvent.click(getByDisplayValue(namespace)); @@ -126,7 +126,7 @@ describe('HeaderBarContent', () => { const { getByTitle } = renderWithRouter(, { handle: { isNamespaced: true, path }, path, - route: `/namespaces/${namespace}/fake/path` + route: path.replace(':namespace', namespace) }); fireEvent.click(getByTitle(/clear selected item/i)); expect(selectNamespace).toHaveBeenCalledWith(ALL_NAMESPACES); @@ -152,14 +152,14 @@ describe('HeaderBarContent', () => { { handle: { isNamespaced: true, path }, path, - route: `/namespaces/${tenantNamespace2}/fake/path` + route: path.replace(':namespace', tenantNamespace2) } ); await waitFor(() => getByDisplayValue(tenantNamespace2)); fireEvent.click(getByTitle(/clear selected item/i)); expect(selectNamespace).toHaveBeenCalledWith(tenantNamespace1); expect(window.location.pathname).toEqual( - `/namespaces/${tenantNamespace1}/fake/path` + path.replace(':namespace', tenantNamespace1) ); }); }); diff --git a/src/containers/NamespacesDropdown/NamespacesDropdown.jsx b/src/containers/NamespacesDropdown/NamespacesDropdown.jsx index 3fa41fb80..e1a5dda17 100644 --- a/src/containers/NamespacesDropdown/NamespacesDropdown.jsx +++ b/src/containers/NamespacesDropdown/NamespacesDropdown.jsx @@ -11,6 +11,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { useMemo } from 'react'; import { useIntl } from 'react-intl'; import { ALL_NAMESPACES } from '@tektoncd/dashboard-utils'; import { TooltipDropdown } from '@tektoncd/dashboard-components'; @@ -55,10 +56,14 @@ const NamespacesDropdown = ({ disableWebSocket: true }); - const selectedItem = { ...originalSelectedItem }; - if (selectedItem && selectedItem.id === ALL_NAMESPACES) { - selectedItem.text = allNamespacesString; - } + const selectedItem = useMemo(() => { + const newSelectedItem = { ...originalSelectedItem }; + if (newSelectedItem.id === ALL_NAMESPACES) { + newSelectedItem.text = allNamespacesString; + } + + return newSelectedItem; + }, [originalSelectedItem?.id]); const items = tenantNamespaces.length ? tenantNamespaces diff --git a/src/containers/NamespacesDropdown/NamespacesDropdown.test.jsx b/src/containers/NamespacesDropdown/NamespacesDropdown.test.jsx index dd13b4555..0beedb938 100644 --- a/src/containers/NamespacesDropdown/NamespacesDropdown.test.jsx +++ b/src/containers/NamespacesDropdown/NamespacesDropdown.test.jsx @@ -47,20 +47,28 @@ it('NamespacesDropdown renders items', () => { }); it('NamespacesDropdown renders controlled selection', () => { + const namespace1 = 'namespace-1'; + const namespace2 = 'namespace-2'; vi.spyOn(API, 'useNamespaces').mockImplementation(() => ({ data: namespaceResources })); // Select item 'namespace-1' const { queryByPlaceholderText, queryByDisplayValue, rerender } = render( - + ); - expect(queryByDisplayValue(/namespace-1/i)).toBeTruthy(); + expect(queryByDisplayValue(namespace1)).toBeTruthy(); // Select item 'namespace-2' render( - , + , { rerender } ); - expect(queryByDisplayValue(/namespace-2/i)).toBeTruthy(); + expect(queryByDisplayValue(namespace2)).toBeTruthy(); // No selected item (select item '') render(, { rerender }); expect(queryByPlaceholderText(initialTextRegExp)).toBeTruthy();