Update one-option Select UX (#3345)

* Update one-option dropdown/select UX * Replace Select with SimpleSelect where applicable * Update tests * Add linter rule and make changes to adapt to the new linter rule
opendatahub-io · Oct 28, 2024 · 2e859cb · 2e859cb
1 parent 105a402
commit 2e859cb
Show file tree

Hide file tree

Showing 29 changed files with 326 additions and 502 deletions.
diff --git a/frontend/.eslintrc b/frontend/.eslintrc
@@ -138,6 +138,11 @@
           {
             "group": ["~/__mocks__/third_party/mlmd", "~/__mocks__/third_party/mlmd/*"],
             "message": "Importing from '~/__mocks__/third_party/mlmd/' is restricted to '~/__mocks__/mlmd/'."
+          },
+          {
+            "group": ["@patternfly/react-core"],
+            "importNames": ["Select"],
+            "message": "Import 'SimpleSelect', 'MultiSelection' or 'TypeaheadSelect' from '~/components' instead."
           }
         ]
       }

diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDeployModal.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry/modelVersionDeployModal.ts
@@ -11,7 +11,7 @@ class ModelVersionDeployModal extends Modal {
 
   selectProjectByName(name: string) {
     this.findProjectSelector().click();
-    this.find().findByRole('menuitem', { name }).click();
+    this.find().findByRole('option', { name }).click();
   }
 }
 

diff --git a/..._tests__/cypress/cypress/tests/mocked/acceleratorProfiles/manageAcceleratorProfiles.cy.ts b/..._tests__/cypress/cypress/tests/mocked/acceleratorProfiles/manageAcceleratorProfiles.cy.ts
@@ -181,8 +181,10 @@ describe('Manage Accelerator Profile', () => {
   it('one preset identifier is auto filled and disabled', () => {
     initIntercepts({});
     identifierAcceleratorProfile.visit();
-    identifierAcceleratorProfile.findIdentifierInput().should('have.value', 'test-identifier');
-    identifierAcceleratorProfile.findIdentifierInput().should('be.disabled');
+    identifierAcceleratorProfile
+      .findAcceleratorIdentifierSelect()
+      .should('contain.text', 'test-identifier');
+    identifierAcceleratorProfile.findAcceleratorIdentifierSelect().should('be.disabled');
   });
 
   it('multiple preset identifiers show dropdown', () => {

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/modelServingGlobal.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/modelServingGlobal.cy.ts
@@ -352,10 +352,12 @@ describe('Model Serving Global', () => {
 
     // test filling in minimum required fields
     inferenceServiceModal.findModelNameInput().type('Test Name');
-    inferenceServiceModal.findServingRuntimeSelect().findSelectOption('OVMS Model Serving').click();
+    inferenceServiceModal.findServingRuntimeSelect().should('contain.text', 'OVMS Model Serving');
+    inferenceServiceModal.findServingRuntimeSelect().should('be.disabled');
     inferenceServiceModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
     inferenceServiceModal.findSubmitButton().should('be.disabled');
-    inferenceServiceModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+    inferenceServiceModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+    inferenceServiceModal.findExistingConnectionSelect().should('be.disabled');
     inferenceServiceModal.findLocationPathInput().type('test-model/');
     inferenceServiceModal.findSubmitButton().should('be.enabled');
     inferenceServiceModal.findNewDataConnectionOption().click();
@@ -436,10 +438,12 @@ describe('Model Serving Global', () => {
 
     // test filling in minimum required fields
     inferenceServiceModal.findModelNameInput().type('trigger-error');
-    inferenceServiceModal.findServingRuntimeSelect().findSelectOption('OVMS Model Serving').click();
+    inferenceServiceModal.findServingRuntimeSelect().should('contain.text', 'OVMS Model Serving');
+    inferenceServiceModal.findServingRuntimeSelect().should('be.disabled');
     inferenceServiceModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
     inferenceServiceModal.findSubmitButton().should('be.disabled');
-    inferenceServiceModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+    inferenceServiceModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+    inferenceServiceModal.findExistingConnectionSelect().should('be.disabled');
     inferenceServiceModal.findLocationPathInput().type('test-model/');
     inferenceServiceModal.findSubmitButton().should('be.enabled');
     inferenceServiceModal.findLocationPathInput().type('test-model/');

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts
@@ -421,7 +421,8 @@ describe('Serving Runtime List', () => {
       inferenceServiceModal.findModelNameInput().type('Test Name');
       inferenceServiceModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
       inferenceServiceModal.findSubmitButton().should('be.disabled');
-      inferenceServiceModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+      inferenceServiceModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+      inferenceServiceModal.findExistingConnectionSelect().should('be.disabled');
       inferenceServiceModal.findLocationPathInput().type('test-model/');
       inferenceServiceModal.findSubmitButton().should('be.enabled');
       inferenceServiceModal.findNewDataConnectionOption().click();
@@ -501,7 +502,7 @@ describe('Serving Runtime List', () => {
       inferenceServiceModalEdit
         .findExistingConnectionSelect()
         .should('have.text', 'Test Secret')
-        .should('be.enabled');
+        .should('be.disabled');
     });
 
     it('ModelMesh ServingRuntime list', () => {
@@ -715,7 +716,8 @@ describe('Serving Runtime List', () => {
       kserveModal.findAuthenticationCheckbox().check();
       kserveModal.findExternalRouteError().should('not.exist');
       kserveModal.findServiceAccountNameInput().should('have.value', 'default-name');
-      kserveModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+      kserveModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+      kserveModal.findExistingConnectionSelect().should('be.disabled');
       kserveModal.findLocationPathInput().type('test-model/');
       kserveModal.findSubmitButton().should('be.enabled');
       kserveModal.findNewDataConnectionOption().click();
@@ -880,7 +882,8 @@ describe('Serving Runtime List', () => {
       kserveModal.findModelNameInput().type('Test Name');
       kserveModal.findServingRuntimeTemplateDropdown().findSelectOption('Caikit').click();
       kserveModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
-      kserveModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+      kserveModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+      kserveModal.findExistingConnectionSelect().should('be.disabled');
       kserveModal.findNewDataConnectionOption().click();
       kserveModal.findLocationNameInput().type('Test Name');
       kserveModal.findLocationAccessKeyInput().type('test-key');
@@ -1105,7 +1108,8 @@ describe('Serving Runtime List', () => {
       kserveModal.findServingRuntimeTemplateDropdown().findSelectOption('Caikit').click();
       kserveModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
       kserveModal.findSubmitButton().should('be.disabled');
-      kserveModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+      kserveModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+      kserveModal.findExistingConnectionSelect().should('be.disabled');
 
       kserveModal.findLocationPathInput().type('test-model/');
       kserveModal.findSubmitButton().should('be.enabled');
@@ -1839,7 +1843,8 @@ describe('Serving Runtime List', () => {
       kserveModal.findServingRuntimeTemplateDropdown().findSelectOption('Caikit').click();
       kserveModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
       kserveModal.findSubmitButton().should('be.disabled');
-      kserveModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+      kserveModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+      kserveModal.findExistingConnectionSelect().should('be.disabled');
       kserveModal.findLocationPathInput().type('test-model/');
       kserveModal.findSubmitButton().should('be.enabled');
       kserveModal.findSubmitButton().click();
@@ -1872,7 +1877,8 @@ describe('Serving Runtime List', () => {
       kserveModal.findServingRuntimeTemplateDropdown().findSelectOption('Caikit').click();
       kserveModal.findModelFrameworkSelect().findSelectOption('onnx - 1').click();
       kserveModal.findSubmitButton().should('be.disabled');
-      kserveModal.findExistingConnectionSelect().findSelectOption('Test Secret').click();
+      kserveModal.findExistingConnectionSelect().should('contain.text', 'Test Secret');
+      kserveModal.findExistingConnectionSelect().should('be.disabled');
       kserveModal.findLocationPathInput().type('test-model/');
       kserveModal.findSubmitButton().should('be.enabled');
       kserveModal.findSubmitButton().click();

diff --git a/frontend/src/components/MultiSelection.tsx b/frontend/src/components/MultiSelection.tsx
@@ -1,5 +1,9 @@
 import * as React from 'react';
 import {
+  /**
+   * The Select component is used to build another generic component here
+   */
+  // eslint-disable-next-line no-restricted-imports
   Select,
   SelectOption,
   SelectList,

diff --git a/frontend/src/components/SimpleSelect.tsx b/frontend/src/components/SimpleSelect.tsx
@@ -2,15 +2,20 @@ import * as React from 'react';
 import {
   Truncate,
   MenuToggle,
+  // eslint-disable-next-line no-restricted-imports
   Select,
   SelectList,
   SelectOption,
   SelectGroup,
   Divider,
   MenuToggleProps,
+  FormHelperText,
+  HelperText,
+  HelperTextItem,
 } from '@patternfly/react-core';
 
 import './SimpleSelect.scss';
+import TruncatedText from '~/components/TruncatedText';
 
 export type SimpleSelectOption = {
   key: string;
@@ -19,6 +24,8 @@ export type SimpleSelectOption = {
   dropdownLabel?: React.ReactNode;
   isPlaceholder?: boolean;
   isDisabled?: boolean;
+  isFavorited?: boolean;
+  dataTestId?: string;
 };
 
 export type SimpleGroupSelectOption = {
@@ -39,6 +46,7 @@ type SimpleSelectProps = {
   isDisabled?: boolean;
   icon?: React.ReactNode;
   dataTestId?: string;
+  previewDescription?: boolean;
 } & Omit<
   React.ComponentProps<typeof Select>,
   'isOpen' | 'toggle' | 'dropdownItems' | 'onChange' | 'selected'
@@ -56,88 +64,140 @@ const SimpleSelect: React.FC<SimpleSelectProps> = ({
   icon,
   dataTestId,
   toggleProps,
+  previewDescription = true,
+  popperProps,
   ...props
 }) => {
   const [open, setOpen] = React.useState(false);
 
+  const groupedOptionsFlat = React.useMemo(
+    () =>
+      groupedOptions?.reduce<SimpleSelectOption[]>((acc, group) => [...acc, ...group.options], []),
+    [groupedOptions],
+  );
+
   const findOptionForKey = (key: string) =>
-    options?.find((option) => option.key === key) ||
-    groupedOptions
-      ?.reduce<SimpleSelectOption[]>((acc, group) => [...acc, ...group.options], [])
-      .find((o) => o.key === key);
+    options?.find((option) => option.key === key) || groupedOptionsFlat?.find((o) => o.key === key);
 
   const selectedOption = value ? findOptionForKey(value) : undefined;
   const selectedLabel = selectedOption?.label ?? placeholder;
 
+  const totalOptions = React.useMemo(
+    () => [...(options || []), ...(groupedOptionsFlat || [])],
+    [options, groupedOptionsFlat],
+  );
+
+  // If there is only one option, call the onChange function
+  const totalOptionsKey = totalOptions.length === 1 ? totalOptions[0].key : null;
+  React.useEffect(() => {
+    if (totalOptionsKey) {
+      onChange(totalOptionsKey, false);
+    }
+    // We don't want the callback function to be a dependency
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [totalOptionsKey]);
+
   return (
-    <Select
-      {...props}
-      isOpen={open}
-      selected={value || toggleLabel}
-      onSelect={(e, selectValue) => {
-        onChange(
-          String(selectValue),
-          !!selectValue && (findOptionForKey(String(selectValue))?.isPlaceholder ?? false),
-        );
-        setOpen(false);
-      }}
-      onOpenChange={setOpen}
-      toggle={(toggleRef) => (
-        <MenuToggle
-          ref={toggleRef}
-          data-testid={dataTestId}
-          aria-label="Options menu"
-          onClick={() => setOpen(!open)}
-          icon={icon}
-          isExpanded={open}
-          isDisabled={isDisabled}
-          isFullWidth={isFullWidth}
-          {...toggleProps}
-        >
-          {toggleLabel || <Truncate content={selectedLabel} className="truncate-no-min-width" />}
-        </MenuToggle>
-      )}
-      shouldFocusToggleOnSelect
-    >
-      {groupedOptions?.map((group, index) => (
-        <>
-          {index > 0 ? <Divider /> : null}
-          <SelectGroup key={group.key} label={group.label}>
-            <SelectList>
-              {group.options.map(
-                ({ key, label, dropdownLabel, description, isDisabled: optionDisabled }) => (
-                  <SelectOption
-                    key={key}
-                    value={key}
-                    description={description}
-                    isDisabled={optionDisabled}
-                    data-testid={key}
-                  >
-                    {dropdownLabel || label}
-                  </SelectOption>
-                ),
-              )}
-            </SelectList>
-          </SelectGroup>
-        </>
-      )) ?? null}
-      {options?.length ? (
-        <SelectList>
-          {groupedOptions?.length ? <Divider /> : null}
-          {options.map(({ key, label, dropdownLabel, description, isDisabled: optionDisabled }) => (
-            <SelectOption
-              key={key}
-              value={key}
-              description={description}
-              isDisabled={optionDisabled}
-              data-testid={key}
-            >
-              {dropdownLabel || label}
-            </SelectOption>
-          ))}
-        </SelectList>
+    <>
+      <Select
+        {...props}
+        isOpen={open}
+        selected={value || toggleLabel}
+        onSelect={(e, selectValue) => {
+          onChange(
+            String(selectValue),
+            !!selectValue && (findOptionForKey(String(selectValue))?.isPlaceholder ?? false),
+          );
+          setOpen(false);
+        }}
+        onOpenChange={setOpen}
+        toggle={(toggleRef) => (
+          <MenuToggle
+            ref={toggleRef}
+            data-testid={dataTestId}
+            aria-label="Options menu"
+            onClick={() => setOpen(!open)}
+            icon={icon}
+            isExpanded={open}
+            isDisabled={totalOptions.length <= 1 || isDisabled}
+            isFullWidth={isFullWidth}
+            {...toggleProps}
+          >
+            {toggleLabel || <Truncate content={selectedLabel} className="truncate-no-min-width" />}
+          </MenuToggle>
+        )}
+        shouldFocusToggleOnSelect
+        popperProps={{ maxWidth: 'trigger', ...popperProps }}
+      >
+        {groupedOptions?.map((group, index) => (
+          <>
+            {index > 0 ? <Divider /> : null}
+            <SelectGroup key={group.key} label={group.label}>
+              <SelectList>
+                {group.options.map(
+                  ({
+                    key,
+                    label,
+                    dropdownLabel,
+                    description,
+                    isFavorited,
+                    isDisabled: optionDisabled,
+                    dataTestId: optionDataTestId,
+                  }) => (
+                    <SelectOption
+                      key={key}
+                      value={key}
+                      description={<TruncatedText maxLines={2} content={description} />}
+                      isDisabled={optionDisabled}
+                      isFavorited={isFavorited}
+                      data-testid={optionDataTestId || key}
+                    >
+                      {dropdownLabel || label}
+                    </SelectOption>
+                  ),
+                )}
+              </SelectList>
+            </SelectGroup>
+          </>
+        )) ?? null}
+        {options?.length ? (
+          <SelectList>
+            {groupedOptions?.length ? <Divider /> : null}
+            {options.map(
+              ({
+                key,
+                label,
+                dropdownLabel,
+                description,
+                isFavorited,
+                isDisabled: optionDisabled,
+                dataTestId: optionDataTestId,
+              }) => (
+                <SelectOption
+                  key={key}
+                  value={key}
+                  description={<TruncatedText maxLines={2} content={description} />}
+                  isFavorited={isFavorited}
+                  isDisabled={optionDisabled}
+                  data-testid={optionDataTestId || key}
+                >
+                  {dropdownLabel || label}
+                </SelectOption>
+              ),
+            )}
+          </SelectList>
+        ) : null}
+      </Select>
+      {previewDescription && selectedOption?.description ? (
+        <FormHelperText>
+          <HelperText>
+            <HelperTextItem>
+              <TruncatedText maxLines={2} content={selectedOption.description} />
+            </HelperTextItem>
+          </HelperText>
+        </FormHelperText>
       ) : null}
-    </Select>
+    </>
   );
 };