Skip to content

Commit

Permalink
Adopt ComboBox selectedItem prop
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
AlanGreene authored and tekton-robot committed Nov 8, 2024
1 parent 663c37b commit 5d90112
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)}
Expand Down
12 changes: 6 additions & 6 deletions src/containers/HeaderBarContent/HeaderBarContent.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('HeaderBarContent', () => {
const path = '/namespaces/:namespace/foo';
renderWithRouter(<HeaderBarContent />, {
path,
route: `/namespaces/${namespace}/foo`
route: path.replace(':namespace', namespace)
});
expect(selectNamespace).toHaveBeenCalledWith(namespace);
});
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -126,7 +126,7 @@ describe('HeaderBarContent', () => {
const { getByTitle } = renderWithRouter(<HeaderBarContent />, {
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);
Expand All @@ -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)
);
});
});
13 changes: 9 additions & 4 deletions src/containers/NamespacesDropdown/NamespacesDropdown.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand Down
16 changes: 12 additions & 4 deletions src/containers/NamespacesDropdown/NamespacesDropdown.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<NamespacesDropdown {...props} selectedItem={{ text: 'namespace-1' }} />
<NamespacesDropdown
{...props}
selectedItem={{ id: namespace1, text: namespace1 }}
/>
);
expect(queryByDisplayValue(/namespace-1/i)).toBeTruthy();
expect(queryByDisplayValue(namespace1)).toBeTruthy();
// Select item 'namespace-2'
render(
<NamespacesDropdown {...props} selectedItem={{ text: 'namespace-2' }} />,
<NamespacesDropdown
{...props}
selectedItem={{ id: namespace2, text: namespace2 }}
/>,
{ rerender }
);
expect(queryByDisplayValue(/namespace-2/i)).toBeTruthy();
expect(queryByDisplayValue(namespace2)).toBeTruthy();
// No selected item (select item '')
render(<NamespacesDropdown {...props} selectedItem="" />, { rerender });
expect(queryByPlaceholderText(initialTextRegExp)).toBeTruthy();
Expand Down

0 comments on commit 5d90112

Please sign in to comment.