From 14b1c4dd7b552e87f912f302b7400d25b2bb03b0 Mon Sep 17 00:00:00 2001 From: Juntao Wang <37624318+DaoDaoNoCode@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:00:47 -0500 Subject: [PATCH] Update typeahead select to make it auto select the only option as needed (#3547) * Update typeahead select to make it auto select the only option as needed * Update function to show description when there is only one option and disabled * fix tests and fix bugs in cluster storage modal --- .../cypress/cypress/pages/clusterStorage.ts | 15 ++- .../cypress/cypress/pages/workbench.ts | 13 ++- .../mocked/projects/clusterStorage.cy.ts | 8 +- .../tests/mocked/projects/workbench.cy.ts | 5 +- frontend/src/components/TypeaheadSelect.tsx | 92 +++++++++++++------ .../connectionTypes/ConnectionTypeForm.tsx | 3 +- .../RegisterModel/RegisteredModelSelector.tsx | 4 +- .../ConnectionSection.tsx | 2 +- .../notebook/ConnectedNotebookField.tsx | 1 + .../detail/storage/ClusterStorageModal.tsx | 68 +++++++------- .../detail/storage/ClusterStorageTableRow.tsx | 7 +- .../projects/screens/detail/storage/utils.ts | 8 ++ .../ExistingDataConnectionField.tsx | 3 +- .../storage/AddExistingStorageField.tsx | 2 +- .../spawner/storage/StorageClassSelect.tsx | 2 +- 15 files changed, 157 insertions(+), 76 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts b/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts index e218cdde6a..4cbcfd458f 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/clusterStorage.ts @@ -77,10 +77,23 @@ class ClusterStorageModal extends Modal { } selectWorkbenchName(row: number, name: string) { - this.findWorkbenchTable().find(`[data-label=Name]`).eq(row).find('button').click(); + this.findWorkbenchSelect(row).click(); cy.findByRole('option', { name, hidden: true }).click(); } + findWorkbenchSelect(row: number) { + return this.findWorkbenchTable() + .find(`[data-label=Name]`) + .eq(row) + .findByTestId('cluster-storage-workbench-select'); + } + + findWorkbenchSelectValueField(row: number) { + return this.findWorkbenchSelect(row).findByRole('combobox', { + name: 'Type to filter', + }); + } + selectCustomPathFormat(row: number) { this.findWorkbenchTable().find(`[data-label="Path format"]`).eq(row).find('button').click(); cy.findByRole('option', { diff --git a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts index 80452d2a4f..44972d670d 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts @@ -353,11 +353,14 @@ class CreateSpawnerPage { return cy.findByTestId('existing-data-connection-type-radio'); } - selectExistingDataConnection(name: string) { - cy.findByTestId('data-connection-group') - .findByRole('button', { name: 'Typeahead menu toggle' }) - .findSelectOption(name) - .click(); + findExistingDataConnectionSelect() { + return cy.findByTestId('existing-data-connection-select'); + } + + findExistingDataConnectionSelectValueField() { + return this.findExistingDataConnectionSelect().findByRole('combobox', { + name: 'Type to filter', + }); } findAwsNameInput() { diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts index 3ca4c6c0c4..f67b28c0c4 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/clusterStorage.cy.ts @@ -205,7 +205,8 @@ describe('ClusterStorage', () => { //connect workbench addClusterStorageModal.findAddWorkbenchButton().click(); addClusterStorageModal.findWorkbenchTable().should('exist'); - addClusterStorageModal.selectWorkbenchName(0, 'test-notebook'); + addClusterStorageModal.findWorkbenchSelect(0).should('have.attr', 'disabled'); + addClusterStorageModal.findWorkbenchSelectValueField(0).should('have.value', 'Test Notebook'); //don't allow duplicate path addClusterStorageModal.findMountPathField(0).fill('test-dupe'); @@ -290,7 +291,10 @@ describe('ClusterStorage', () => { const clusterStorageRow = clusterStorage.getClusterStorageRow('Existing PVC'); clusterStorageRow.findKebabAction('Edit storage').click(); updateClusterStorageModal.findAddWorkbenchButton().click(); - updateClusterStorageModal.selectWorkbenchName(1, 'another-notebook'); + addClusterStorageModal.findWorkbenchSelect(1).should('have.attr', 'disabled'); + addClusterStorageModal + .findWorkbenchSelectValueField(1) + .should('have.value', 'Another Notebook'); updateClusterStorageModal.findMountPathField(1).fill('new-data'); cy.interceptK8s('PATCH', NotebookModel, anotherNotebook).as('updateClusterStorage'); diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts index 97839020ee..894f6b090d 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts @@ -319,7 +319,10 @@ describe('Workbench page', () => { //add existing data connection createSpawnerPage.findExistingDataConnectionRadio().click(); - createSpawnerPage.selectExistingDataConnection('Test Secret'); + createSpawnerPage.findExistingDataConnectionSelect().should('have.attr', 'disabled'); + createSpawnerPage + .findExistingDataConnectionSelectValueField() + .should('have.value', 'Test Secret'); createSpawnerPage.findSubmitButton().click(); cy.wait('@createConfigMap').then((interception) => { diff --git a/frontend/src/components/TypeaheadSelect.tsx b/frontend/src/components/TypeaheadSelect.tsx index 74c20472d5..0665b1508c 100644 --- a/frontend/src/components/TypeaheadSelect.tsx +++ b/frontend/src/components/TypeaheadSelect.tsx @@ -16,8 +16,12 @@ import { Button, MenuToggleProps, SelectProps, + FormHelperText, + HelperTextItem, + HelperText, } from '@patternfly/react-core'; import { TimesIcon } from '@patternfly/react-icons'; +import TruncatedText from '~/components/TruncatedText'; export interface TypeaheadSelectOption extends Omit { /** Content of the select option. */ @@ -70,6 +74,12 @@ export interface TypeaheadSelectProps extends Omit `No results found for "${filter}"`; @@ -95,6 +105,9 @@ const TypeaheadSelect: React.FunctionComponent = ({ isDisabled, toggleWidth, toggleProps, + isRequired = true, + dataTestId, + previewDescription = true, ...props }: TypeaheadSelectProps) => { const [isOpen, setIsOpen] = React.useState(false); @@ -234,6 +247,19 @@ const TypeaheadSelect: React.FunctionComponent = ({ closeMenu(); }; + const notAllowEmpty = !isCreatable && isRequired; + // Only when the field is required, not creatable and there is one option, we auto select the first option + const isSingleOption = selectOptions.length === 1 && notAllowEmpty; + const singleOptionValue = isSingleOption ? selectOptions[0].value : null; + // If there is only one option, call the onChange function + React.useEffect(() => { + if (singleOptionValue && onSelect) { + onSelect(undefined, singleOptionValue); + } + // We don't want the callback function to be a dependency + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [singleOptionValue]); + const handleSelect = ( _event: React.MouseEvent | undefined, value: string | number | undefined, @@ -363,9 +389,10 @@ const TypeaheadSelect: React.FunctionComponent = ({ ref={toggleRef} variant="typeahead" aria-label="Typeahead menu toggle" + data-testid={dataTestId} onClick={onToggleClick} isExpanded={isOpen} - isDisabled={isDisabled} + isDisabled={isDisabled || (selectOptions.length <= 1 && notAllowEmpty)} isFullWidth style={{ width: toggleWidth }} {...toggleProps} @@ -399,32 +426,43 @@ const TypeaheadSelect: React.FunctionComponent = ({ ); return ( - + <> + + {previewDescription && isSingleOption && selected?.description ? ( + + + + + + + + ) : null} + ); }; diff --git a/frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx b/frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx index 3d22d5bc7c..c67d00c2ac 100644 --- a/frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx +++ b/frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx @@ -146,7 +146,7 @@ const ConnectionTypeForm: React.FC = ({ setConnectionType?.(selection); } }} - isDisabled={isPreview || !options || options.length <= 1} + isDisabled={isPreview || !options} placeholder={ isPreview && !connectionType?.metadata.annotations?.['openshift.io/display-name'] ? 'Unspecified' @@ -166,6 +166,7 @@ const ConnectionTypeForm: React.FC = ({ String(o.data).toLowerCase().includes(filterValue.toLowerCase()), ) } + previewDescription={false} /> {connectionType && ( diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisteredModelSelector.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisteredModelSelector.tsx index 4516a87426..d81a865530 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisteredModelSelector.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisteredModelSelector.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { TextInput } from '@patternfly/react-core'; -import { TypeaheadSelect, TypeaheadSelectOption } from '@patternfly/react-templates'; import { RegisteredModel } from '~/concepts/modelRegistry/types'; +import TypeaheadSelect, { TypeaheadSelectOption } from '~/components/TypeaheadSelect'; type RegisteredModelSelectorProps = { registeredModels: RegisteredModel[]; @@ -49,7 +49,7 @@ const RegisteredModelSelector: React.FC = ({ setRegisteredModelId('')} - initialOptions={options} + selectOptions={options} placeholder="Select a registered model" noOptionsFoundMessage={(filter) => `No results found for "${filter}"`} onSelect={(_event, selection) => { diff --git a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx index 94e16916c0..6f96a543a6 100644 --- a/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx +++ b/frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx @@ -135,8 +135,8 @@ const ExistingConnectionField: React.FC = ({ onSelect(newConnection); } }} - isDisabled={projectConnections.length === 0} popperProps={{ appendTo: 'inline' }} + previewDescription={false} /> {selectedConnection && ( = ({ id: 'notebook-search-input', }} data-testid="notebook-search-select" + isRequired={false} /> )} diff --git a/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx b/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx index 9c705aa5ed..a1a8c7e429 100644 --- a/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx +++ b/frontend/src/pages/projects/screens/detail/storage/ClusterStorageModal.tsx @@ -1,7 +1,7 @@ import * as React from 'react'; import { Button, Stack, FormGroup, StackItem } from '@patternfly/react-core'; import { PlusCircleIcon } from '@patternfly/react-icons'; -import isEqual from 'lodash-es/isEqual'; +import { isEqual } from 'lodash-es'; import { NotebookKind, PersistentVolumeClaimKind } from '~/k8sTypes'; import { ClusterStorageNotebookSelection, StorageData } from '~/pages/projects/types'; import { ProjectDetailsContext } from '~/pages/projects/ProjectDetailsContext'; @@ -31,6 +31,7 @@ import { isPvcUpdateRequired, getInUseMountPaths, validateClusterMountPath, + getDefaultMountPathFromStorageName, } from './utils'; import { storageColumns } from './clusterTableColumns'; import ClusterStorageTableRow from './ClusterStorageTableRow'; @@ -94,7 +95,7 @@ const ClusterStorageModal: React.FC = ({ existingPvc, { name: '', mountPath: { - value: MOUNT_PATH_PREFIX, + value: getDefaultMountPathFromStorageName(storageName, newRowId), error: '', }, existingPvc: false, @@ -103,7 +104,7 @@ const ClusterStorageModal: React.FC = ({ existingPvc, }, ]); setNewRowId((prev) => prev + 1); - }, [newRowId, notebookData]); + }, [newRowId, notebookData, storageName]); const { updatedNotebooks, removedNotebooks } = handleConnectedNotebooks( notebookData, @@ -192,34 +193,38 @@ const ClusterStorageModal: React.FC = ({ existingPvc, const isValid = hasAllValidNotebookRelationship; - React.useEffect(() => { - const updatedData = notebookData.map((notebook) => { - if (!notebook.existingPvc && !notebook.isUpdatedValue && storageName) { - const defaultPathValue = `${MOUNT_PATH_PREFIX}${storageName - .toLowerCase() - .replace(/\s+/g, '-')}-${notebook.newRowId ?? 1}`; - - const inUseMountPaths = getInUseMountPaths( - notebook.name, - allNotebooks, - existingPvc?.metadata.name, - ); - - return { - ...notebook, - mountPath: { - value: defaultPathValue, - error: validateClusterMountPath(defaultPathValue, inUseMountPaths), - }, - }; + const updatePathWithNameChange = React.useCallback( + (newStorageName: string) => { + const updatedData = notebookData.map((notebook) => { + if (!notebook.existingPvc && !notebook.isUpdatedValue) { + const defaultPathValue = getDefaultMountPathFromStorageName( + newStorageName, + notebook.newRowId, + ); + + const inUseMountPaths = getInUseMountPaths( + notebook.name, + allNotebooks, + existingPvc?.metadata.name, + ); + + return { + ...notebook, + mountPath: { + value: defaultPathValue, + error: validateClusterMountPath(defaultPathValue, inUseMountPaths), + }, + }; + } + return notebook; + }); + + if (!isEqual(updatedData, notebookData)) { + setNotebookData(updatedData); } - return notebook; - }); - - if (!isEqual(updatedData, notebookData)) { - setNotebookData(updatedData); - } - }, [allNotebooks, existingPvc, notebookData, storageName]); + }, + [allNotebooks, existingPvc?.metadata.name, notebookData], + ); const handleMountPathUpdate = React.useCallback( (rowIndex: number, value: string, format: MountPathFormat) => { @@ -265,6 +270,7 @@ const ClusterStorageModal: React.FC = ({ existingPvc, } onNameChange={(currentName) => { setStorageName(currentName); + updatePathWithNameChange(currentName); }} submitLabel={existingPvc ? 'Update' : 'Add storage'} isValid={isValid} @@ -283,7 +289,7 @@ const ClusterStorageModal: React.FC = ({ existingPvc, data={notebookData} rowRenderer={(row, rowIndex) => ( = ({ const selectOptions = availableNotebooks.notebooks.map((notebook) => ({ value: notebook.metadata.name, - content: notebook.metadata.name, + content: getDisplayNameFromK8sResource(notebook), })); let placeholderText: string; @@ -82,7 +83,7 @@ const ClusterStorageTableRow: React.FC = ({ onNotebookSelect('')} @@ -91,6 +92,8 @@ const ClusterStorageTableRow: React.FC = ({ onNotebookSelect(selectedValue); } }} + previewDescription={false} + dataTestId="cluster-storage-workbench-select" /> ) : ( <> diff --git a/frontend/src/pages/projects/screens/detail/storage/utils.ts b/frontend/src/pages/projects/screens/detail/storage/utils.ts index 5e0ed6ce75..162a318b69 100644 --- a/frontend/src/pages/projects/screens/detail/storage/utils.ts +++ b/frontend/src/pages/projects/screens/detail/storage/utils.ts @@ -106,3 +106,11 @@ export const getInUseMountPaths = ( .filter((key) => key !== existingPvcName) .map((key) => allInUseMountPaths[key]); }; + +export const getDefaultMountPathFromStorageName = ( + storageName?: string, + newRowId?: number, +): string => + storageName + ? `${MOUNT_PATH_PREFIX}${storageName.toLowerCase().replace(/\s+/g, '-')}-${newRowId ?? 1}` + : MOUNT_PATH_PREFIX; diff --git a/frontend/src/pages/projects/screens/spawner/dataConnection/ExistingDataConnectionField.tsx b/frontend/src/pages/projects/screens/spawner/dataConnection/ExistingDataConnectionField.tsx index bd932e9478..d8d91de645 100644 --- a/frontend/src/pages/projects/screens/spawner/dataConnection/ExistingDataConnectionField.tsx +++ b/frontend/src/pages/projects/screens/spawner/dataConnection/ExistingDataConnectionField.tsx @@ -57,13 +57,14 @@ const ExistingDataConnectionField: React.FC = > setDataConnection(String(selection))} onClearSelection={() => setDataConnection()} placeholder={placeholderText} noOptionsFoundMessage={(filter) => `No data connection was found for "${filter}"`} - isDisabled={!loaded || connections.length === 0} + isDisabled={!loaded} /> ); diff --git a/frontend/src/pages/projects/screens/spawner/storage/AddExistingStorageField.tsx b/frontend/src/pages/projects/screens/spawner/storage/AddExistingStorageField.tsx index 9c38777f50..5b1945079b 100644 --- a/frontend/src/pages/projects/screens/spawner/storage/AddExistingStorageField.tsx +++ b/frontend/src/pages/projects/screens/spawner/storage/AddExistingStorageField.tsx @@ -92,7 +92,7 @@ const AddExistingStorageField: React.FC = ({ placeholder={placeholderText} noOptionsFoundMessage={(filter) => `No persistent storage was found for "${filter}"`} popperProps={{ direction: selectDirection, appendTo: menuAppendTo }} - isDisabled={!loaded || storages.length === 0} + isDisabled={!loaded} data-testid="persistent-storage-typeahead" /> diff --git a/frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx b/frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx index d62b91805b..b24848b32b 100644 --- a/frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx +++ b/frontend/src/pages/projects/screens/spawner/storage/StorageClassSelect.tsx @@ -34,7 +34,7 @@ const StorageClassSelect: React.FC = ({ const [defaultSc] = useDefaultStorageClass(); const enabledStorageClasses = storageClasses - // .filter((sc) => getStorageClassConfig(sc)?.isEnabled === true) + .filter((sc) => getStorageClassConfig(sc)?.isEnabled === true) .toSorted((a, b) => { const aConfig = getStorageClassConfig(a); const bConfig = getStorageClassConfig(b);