Update typeahead select to make it auto select the only option as nee…

…ded (#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
opendatahub-io · Dec 18, 2024 · 14b1c4d · 14b1c4d
1 parent 6a0bc4a
commit 14b1c4d
Show file tree

Hide file tree

Showing 15 changed files with 157 additions and 76 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<SelectOptionProps, 'content' | 'isSelected'> {
   /** Content of the select option. */
@@ -70,6 +74,12 @@ export interface TypeaheadSelectProps extends Omit<SelectProps, 'toggle' | 'onSe
   toggleWidth?: string;
   /** Additional props passed to the toggle. */
   toggleProps?: MenuToggleProps;
+  /** Flag to indicate if the selection is required or not */
+  isRequired?: boolean;
+  /** Test id of the toggle */
+  dataTestId?: string;
+  /** Flag to indicate if showing the description under the toggle */
+  previewDescription?: boolean;
 }
 
 const defaultNoOptionsFoundMessage = (filter: string) => `No results found for "${filter}"`;
@@ -95,6 +105,9 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
   isDisabled,
   toggleWidth,
   toggleProps,
+  isRequired = true,
+  dataTestId,
+  previewDescription = true,
   ...props
 }: TypeaheadSelectProps) => {
   const [isOpen, setIsOpen] = React.useState(false);
@@ -234,6 +247,19 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
     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<Element, MouseEvent> | undefined,
     value: string | number | undefined,
@@ -363,9 +389,10 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
       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<TypeaheadSelectProps> = ({
   );
 
   return (
-    <Select
-      isOpen={isOpen}
-      selected={selected}
-      onSelect={handleSelect}
-      onOpenChange={(open) => !open && closeMenu()}
-      toggle={toggle}
-      shouldFocusFirstItemOnOpen={false}
-      ref={innerRef}
-      {...props}
-    >
-      <SelectList>
-        {filteredSelections.map((option, index) => {
-          const { content, value, ...optionProps } = option;
-          return (
-            <SelectOption
-              key={value}
-              value={value}
-              isFocused={focusedItemIndex === index}
-              {...optionProps}
-            >
-              {content}
-            </SelectOption>
-          );
-        })}
-      </SelectList>
-    </Select>
+    <>
+      <Select
+        isOpen={isOpen}
+        selected={selected}
+        onSelect={handleSelect}
+        onOpenChange={(open) => !open && closeMenu()}
+        toggle={toggle}
+        shouldFocusFirstItemOnOpen={false}
+        ref={innerRef}
+        {...props}
+      >
+        <SelectList>
+          {filteredSelections.map((option, index) => {
+            const { content, value, ...optionProps } = option;
+            return (
+              <SelectOption
+                key={value}
+                value={value}
+                isFocused={focusedItemIndex === index}
+                {...optionProps}
+              >
+                {content}
+              </SelectOption>
+            );
+          })}
+        </SelectList>
+      </Select>
+      {previewDescription && isSingleOption && selected?.description ? (
+        <FormHelperText>
+          <HelperText>
+            <HelperTextItem>
+              <TruncatedText maxLines={2} content={selected.description} />
+            </HelperTextItem>
+          </HelperText>
+        </FormHelperText>
+      ) : null}
+    </>
   );
 };
 

diff --git a/frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx b/frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx
@@ -146,7 +146,7 @@ const ConnectionTypeForm: React.FC<Props> = ({
               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<Props> = ({
                 String(o.data).toLowerCase().includes(filterValue.toLowerCase()),
             )
           }
+          previewDescription={false}
         />
         {connectionType && (
           <ConnectionTypeDetailsHelperText connectionType={connectionType} isPreview={isPreview} />

diff --git 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<RegisteredModelSelectorProps> = ({
     <TypeaheadSelect
       id="model-name"
       onClearSelection={() => 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
@@ -135,8 +135,8 @@ const ExistingConnectionField: React.FC<ExistingConnectionFieldProps> = ({
               onSelect(newConnection);
             }
           }}
-          isDisabled={projectConnections.length === 0}
           popperProps={{ appendTo: 'inline' }}
+          previewDescription={false}
         />
         {selectedConnection && (
           <ConnectionDetailsHelperText

diff --git a/frontend/src/pages/projects/notebook/ConnectedNotebookField.tsx b/frontend/src/pages/projects/notebook/ConnectedNotebookField.tsx
@@ -89,6 +89,7 @@ const ConnectedNotebookField: React.FC<SelectNotebookFieldProps> = ({
             id: 'notebook-search-input',
           }}
           data-testid="notebook-search-select"
+          isRequired={false}
         />
       )}
       <FormHelperText>

diff --git 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<ClusterStorageModalProps> = ({ existingPvc,
       {
         name: '',
         mountPath: {
-          value: MOUNT_PATH_PREFIX,
+          value: getDefaultMountPathFromStorageName(storageName, newRowId),
           error: '',
         },
         existingPvc: false,
@@ -103,7 +104,7 @@ const ClusterStorageModal: React.FC<ClusterStorageModalProps> = ({ existingPvc,
       },
     ]);
     setNewRowId((prev) => prev + 1);
-  }, [newRowId, notebookData]);
+  }, [newRowId, notebookData, storageName]);
 
   const { updatedNotebooks, removedNotebooks } = handleConnectedNotebooks(
     notebookData,
@@ -192,34 +193,38 @@ const ClusterStorageModal: React.FC<ClusterStorageModalProps> = ({ 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<ClusterStorageModalProps> = ({ existingPvc,
       }
       onNameChange={(currentName) => {
         setStorageName(currentName);
+        updatePathWithNameChange(currentName);
       }}
       submitLabel={existingPvc ? 'Update' : 'Add storage'}
       isValid={isValid}
@@ -283,7 +289,7 @@ const ClusterStorageModal: React.FC<ClusterStorageModalProps> = ({ existingPvc,
                   data={notebookData}
                   rowRenderer={(row, rowIndex) => (
                     <ClusterStorageTableRow
-                      key={rowIndex}
+                      key={row.newRowId || row.name}
                       obj={row}
                       inUseMountPaths={getInUseMountPaths(
                         row.name,