From 47349ea3e7d879fbcfc0e8d827262bde7723a065 Mon Sep 17 00:00:00 2001 From: Juntao Wang Date: Tue, 22 Oct 2024 16:04:45 -0400 Subject: [PATCH] Add linter rule and make changes to adapt to the new linter rule --- frontend/.eslintrc | 5 + .../modelServing/servingRuntimeList.cy.ts | 2 +- frontend/src/components/MultiSelection.tsx | 4 + frontend/src/components/SimpleSelect.tsx | 7 + frontend/src/components/TypeaheadSelect.tsx | 4 + .../fields/DropdownFormField.tsx | 15 +- .../PipelineRunArtifactSelect.tsx | 5 + .../AcceleratorIdentifierMultiselect.tsx | 191 +++--------------- .../screens/ModelRegistrySelector.tsx | 121 ++++------- 9 files changed, 102 insertions(+), 252 deletions(-) diff --git a/frontend/.eslintrc b/frontend/.eslintrc index 5120c919b5..41680a0887 100755 --- 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/tests/mocked/modelServing/servingRuntimeList.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts index c4c8f4186d..8c04e31a04 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts @@ -502,7 +502,7 @@ describe('Serving Runtime List', () => { inferenceServiceModalEdit .findExistingConnectionSelect() .should('have.text', 'Test Secret') - .should('be.enabled'); + .should('be.disabled'); }); it('ModelMesh ServingRuntime list', () => { diff --git a/frontend/src/components/MultiSelection.tsx b/frontend/src/components/MultiSelection.tsx index 666e180bc9..772425fa81 100644 --- 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 index 83de18ac6f..ec123cce3a 100644 --- a/frontend/src/components/SimpleSelect.tsx +++ b/frontend/src/components/SimpleSelect.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import { Truncate, MenuToggle, + // eslint-disable-next-line no-restricted-imports Select, SelectList, SelectOption, @@ -23,6 +24,7 @@ export type SimpleSelectOption = { dropdownLabel?: React.ReactNode; isPlaceholder?: boolean; isDisabled?: boolean; + isFavorited?: boolean; dataTestId?: string; }; @@ -91,6 +93,7 @@ const SimpleSelect: React.FC = ({ 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]); @@ -137,6 +140,7 @@ const SimpleSelect: React.FC = ({ label, dropdownLabel, description, + isFavorited, isDisabled: optionDisabled, dataTestId: optionDataTestId, }) => ( @@ -145,6 +149,7 @@ const SimpleSelect: React.FC = ({ value={key} description={} isDisabled={optionDisabled} + isFavorited={isFavorited} data-testid={optionDataTestId || key} > {dropdownLabel || label} @@ -164,6 +169,7 @@ const SimpleSelect: React.FC = ({ label, dropdownLabel, description, + isFavorited, isDisabled: optionDisabled, dataTestId: optionDataTestId, }) => ( @@ -171,6 +177,7 @@ const SimpleSelect: React.FC = ({ key={key} value={key} description={} + isFavorited={isFavorited} isDisabled={optionDisabled} data-testid={optionDataTestId || key} > diff --git a/frontend/src/components/TypeaheadSelect.tsx b/frontend/src/components/TypeaheadSelect.tsx index 1d9aab25c3..fe89450e8d 100644 --- a/frontend/src/components/TypeaheadSelect.tsx +++ b/frontend/src/components/TypeaheadSelect.tsx @@ -1,5 +1,9 @@ import 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/concepts/connectionTypes/fields/DropdownFormField.tsx b/frontend/src/concepts/connectionTypes/fields/DropdownFormField.tsx index 0143c97be4..bf8db6c7b5 100644 --- a/frontend/src/concepts/connectionTypes/fields/DropdownFormField.tsx +++ b/frontend/src/concepts/connectionTypes/fields/DropdownFormField.tsx @@ -1,5 +1,18 @@ import * as React from 'react'; -import { Badge, MenuToggle, Select, SelectList, SelectOption } from '@patternfly/react-core'; + +import { + Badge, + MenuToggle, + /** + * This is a special use case to use the Select component to dynamically generate either single/multi dropdown component + * And it allows user to de-select options + * No need to replace it with SimpleSelect or MultiSelection component here + */ + // eslint-disable-next-line no-restricted-imports + Select, + SelectList, + SelectOption, +} from '@patternfly/react-core'; import { DropdownField } from '~/concepts/connectionTypes/types'; import { FieldProps } from '~/concepts/connectionTypes/fields/types'; import DefaultValueTextRenderer from '~/concepts/connectionTypes/fields/DefaultValueTextRenderer'; diff --git a/frontend/src/concepts/pipelines/content/compareRuns/metricsSection/PipelineRunArtifactSelect.tsx b/frontend/src/concepts/pipelines/content/compareRuns/metricsSection/PipelineRunArtifactSelect.tsx index 39367e9c3d..bf68cbc125 100644 --- a/frontend/src/concepts/pipelines/content/compareRuns/metricsSection/PipelineRunArtifactSelect.tsx +++ b/frontend/src/concepts/pipelines/content/compareRuns/metricsSection/PipelineRunArtifactSelect.tsx @@ -1,5 +1,10 @@ import React from 'react'; import { + /** Special use case for Select in this file + * It allows multi-selection in the dropdown while keeps the toggle unchanged + * Don't use SimpleSelect here + */ + // eslint-disable-next-line no-restricted-imports Select, SelectOption, SelectList, diff --git a/frontend/src/pages/BYONImages/BYONImageModal/AcceleratorIdentifierMultiselect.tsx b/frontend/src/pages/BYONImages/BYONImageModal/AcceleratorIdentifierMultiselect.tsx index 5e646a6c60..ec6b79d13b 100644 --- a/frontend/src/pages/BYONImages/BYONImageModal/AcceleratorIdentifierMultiselect.tsx +++ b/frontend/src/pages/BYONImages/BYONImageModal/AcceleratorIdentifierMultiselect.tsx @@ -1,21 +1,7 @@ -import React, { useEffect, useState } from 'react'; -import { - Button, - Chip, - ChipGroup, - MenuToggle, - MenuToggleElement, - Select, - SelectList, - SelectOption, - SelectOptionProps, - TextInputGroup, - TextInputGroupMain, - TextInputGroupUtilities, -} from '@patternfly/react-core'; -import { TimesIcon } from '@patternfly/react-icons'; +import React from 'react'; import useAcceleratorProfiles from '~/pages/notebookController/screens/server/useAcceleratorProfiles'; import { useDashboardNamespace } from '~/redux/selectors'; +import { MultiSelection } from '~/components/MultiSelection'; type AcceleratorIdentifierMultiselectProps = { data: string[]; @@ -27,162 +13,31 @@ export const AcceleratorIdentifierMultiselect: React.FC { const { dashboardNamespace } = useDashboardNamespace(); - const [acceleratorProfiles, loaded, loadError] = useAcceleratorProfiles(dashboardNamespace); + const [acceleratorProfiles] = useAcceleratorProfiles(dashboardNamespace); - const [isOpen, setIsOpen] = React.useState(false); - const [inputValue, setInputValue] = React.useState(''); - const [selectOptions, setSelectOptions] = useState([]); - const [onCreation, setOnCreation] = React.useState(false); // Boolean to refresh filter state after new option is created + const identifiers = React.useMemo(() => { + const uniqueIdentifiers = new Set(data); - const textInputRef = React.useRef(); + // Add identifiers from accelerators + acceleratorProfiles.forEach((cr) => { + uniqueIdentifiers.add(cr.spec.identifier); + }); - useEffect(() => { - if (loaded && !loadError) { - const uniqueIdentifiers = new Set(); - - // Add identifiers from accelerators - acceleratorProfiles.forEach((cr) => { - uniqueIdentifiers.add(cr.spec.identifier); - }); - - // Add identifiers from initial data - data.forEach((identifier) => { - uniqueIdentifiers.add(identifier); - }); - - // Convert unique identifiers to SelectOptionProps - let newOptions = Array.from(uniqueIdentifiers).map((identifier) => ({ - value: identifier, - children: identifier, - })); - - // Filter menu items based on the text input value when one exists - if (inputValue) { - newOptions = newOptions.filter((menuItem) => - String(menuItem.children).toLowerCase().includes(inputValue.toLowerCase()), - ); - - // When no options are found after filtering, display creation option - if (!newOptions.length) { - newOptions = [{ children: `Create new option "${inputValue}"`, value: 'create' }]; - } - - // Open the menu when the input value changes and the new value is not empty - if (!isOpen) { - setIsOpen(true); - } - } - - setSelectOptions(newOptions); - } - }, [acceleratorProfiles, loaded, loadError, data, onCreation, inputValue, isOpen]); - - const onToggleClick = () => { - setIsOpen(!isOpen); - }; - - const onTextInputChange = (_event: React.FormEvent, value: string) => { - setInputValue(value); - }; - - const onSelect = (value: string) => { - if (value) { - if (value === 'create') { - // Check if the input value already exists in selectOptions - if (!selectOptions.some((option) => option.value === inputValue)) { - // Add the new option to selectOptions - setSelectOptions([...selectOptions, { value: inputValue, children: inputValue }]); - } - // Update the selected values - setData( - data.includes(inputValue) - ? data.filter((selection) => selection !== inputValue) - : [...data, inputValue], - ); - setOnCreation(!onCreation); - setInputValue(''); - } else { - // Handle selecting an existing option - setData( - data.includes(value) ? data.filter((selection) => selection !== value) : [...data, value], - ); - } - } - textInputRef.current?.focus(); - }; - - const toggle = (toggleRef: React.Ref) => ( - - - - - {data.map((selection, index) => ( - { - ev.stopPropagation(); - onSelect(selection); - }} - > - {selection} - - ))} - - - - {data.length > 0 && ( - - )} - - - - ); + return Array.from(uniqueIdentifiers); + }, [acceleratorProfiles, data]); return ( - + ({ + id: identifier, + name: identifier, + selected: data.includes(identifier), + }))} + setValue={(newState) => setData(newState.filter((n) => n.selected).map((n) => String(n.id)))} + placeholder="Example, nvidia.com/gpu" + isCreatable + createOptionMessage={(newValue) => `Create new option "${newValue}"`} + /> ); }; diff --git a/frontend/src/pages/modelRegistry/screens/ModelRegistrySelector.tsx b/frontend/src/pages/modelRegistry/screens/ModelRegistrySelector.tsx index 5f54484c7c..a07c0f7c55 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelRegistrySelector.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelRegistrySelector.tsx @@ -6,28 +6,19 @@ import { DescriptionListDescription, DescriptionListGroup, DescriptionListTerm, - Divider, Flex, FlexItem, Icon, - MenuToggle, Popover, - Select, - SelectGroup, - SelectList, - SelectOption, Tooltip, } from '@patternfly/react-core'; import truncateStyles from '@patternfly/react-styles/css/components/Truncate/truncate'; import { InfoCircleIcon, BlueprintIcon } from '@patternfly/react-icons'; import { useBrowserStorage } from '~/components/browserStorage'; import { ModelRegistrySelectorContext } from '~/concepts/modelRegistry/context/ModelRegistrySelectorContext'; -import { - getDescriptionFromK8sResource, - getDisplayNameFromK8sResource, - getResourceNameFromK8sResource, -} from '~/concepts/k8s/utils'; +import { getDescriptionFromK8sResource, getDisplayNameFromK8sResource } from '~/concepts/k8s/utils'; import { ServiceKind } from '~/k8sTypes'; +import SimpleSelect, { SimpleSelectOption } from '~/components/SimpleSelect'; const MODEL_REGISTRY_FAVORITE_STORAGE_KEY = 'odh.dashboard.model.registry.favorite'; @@ -46,7 +37,6 @@ const ModelRegistrySelector: React.FC = ({ ModelRegistrySelectorContext, ); const selection = modelRegistryServices.find((mr) => mr.metadata.name === modelRegistry); - const [isOpen, setIsOpen] = React.useState(false); const [favorites, setFavorites] = useBrowserStorage( MODEL_REGISTRY_FAVORITE_STORAGE_KEY, [], @@ -81,72 +71,49 @@ const ModelRegistrySelector: React.FC = ({ ); }; - const options = [ - - - {modelRegistryServices.map((mr) => ( - - {getDisplayNameFromK8sResource(mr)} - - ))} - - , - ]; - - const createFavorites = (favIds: string[]) => { - const favorite: JSX.Element[] = []; + const allOptions: SimpleSelectOption[] = modelRegistryServices.map((mr) => ({ + key: mr.metadata.name, + label: mr.metadata.name, + dropdownLabel: getDisplayNameFromK8sResource(mr), + description: getMRSelectDescription(mr), + isFavorited: favorites.includes(mr.metadata.name), + })); - options.forEach((item) => { - if (item.type === SelectList) { - item.props.children.filter( - (child: JSX.Element) => favIds.includes(child.props.value) && favorite.push(child), - ); - } else if (item.type === SelectGroup) { - item.props.children.props.children.filter( - (child: JSX.Element) => favIds.includes(child.props.value) && favorite.push(child), - ); - } else if (favIds.includes(item.props.value)) { - favorite.push(item); - } - }); - - return favorite; - }; + const favoriteOptions = (favIds: string[]) => + allOptions.filter((option) => favIds.includes(option.key)); const selector = ( - + /> ); if (primary) {