From 11aa082d5d9aace2a4d74cae999218ea2bfa0ead Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Wed, 30 Oct 2024 16:34:18 -0400 Subject: [PATCH 01/15] saving --- .../DashboardDescriptionListGroup.tsx | 1 + .../EditableLabelsDescriptionListGroup.tsx | 277 +++++++++--------- .../ModelVersionDetailsView.tsx | 4 +- .../ModelVersions/ModelDetailsView.tsx | 4 +- 4 files changed, 138 insertions(+), 148 deletions(-) diff --git a/frontend/src/components/DashboardDescriptionListGroup.tsx b/frontend/src/components/DashboardDescriptionListGroup.tsx index 607f3b0920..e294be826d 100644 --- a/frontend/src/components/DashboardDescriptionListGroup.tsx +++ b/frontend/src/components/DashboardDescriptionListGroup.tsx @@ -36,6 +36,7 @@ export type DashboardDescriptionListGroupProps = { contentWhenEmpty?: React.ReactNode; children: React.ReactNode; groupTestId?: string; + isSaveDisabled?: boolean; } & (({ isEditable: true } & EditableProps) | ({ isEditable?: false } & Partial)); const DashboardDescriptionListGroup: React.FC = (props) => { diff --git a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx index 309a4dca9b..970f74bd5d 100644 --- a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx +++ b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx @@ -1,73 +1,90 @@ -import * as React from 'react'; +import React, { useState } from 'react'; import { - Button, - Form, - FormGroup, - FormHelperText, - HelperText, - HelperTextItem, Label, LabelGroup, - Modal, - TextInput, + Alert, + AlertVariant } from '@patternfly/react-core'; -import { ExclamationCircleIcon } from '@patternfly/react-icons'; -import DashboardDescriptionListGroup, { - DashboardDescriptionListGroupProps, -} from './DashboardDescriptionListGroup'; +import DashboardDescriptionListGroup from './DashboardDescriptionListGroup'; -type EditableTextDescriptionListGroupProps = Partial< - Pick -> & { +interface EditableLabelsProps { labels: string[]; - saveEditedLabels: (labels: string[]) => Promise; - allExistingKeys?: string[]; + onLabelsChange: (labels: string[]) => Promise; isArchive?: boolean; -}; + allExistingKeys?: string[]; + title?: string; + contentWhenEmpty?: string; +} -const EditableLabelsDescriptionListGroup: React.FC = ({ +export const EditableLabelsDescriptionListGroup: React.FC = ({ title = 'Labels', contentWhenEmpty = 'No labels', labels, - saveEditedLabels, + onLabelsChange, isArchive, allExistingKeys = labels, }) => { - const [isEditing, setIsEditing] = React.useState(false); - const [unsavedLabels, setUnsavedLabels] = React.useState(labels); - const [isSavingEdits, setIsSavingEdits] = React.useState(false); + const [isEditing, setIsEditing] = useState(false); + const [isSavingEdits, setIsSavingEdits] = useState(false); + const [unsavedLabels, setUnsavedLabels] = useState(labels); + const [addLabelInputValue, setAddLabelInputValue] = useState(''); + const [isAddLabelModalOpen, setIsAddLabelModalOpen] = useState(false); + const [labelErrors, setLabelErrors] = useState<{ [key: string]: string }>({}); - const editUnsavedLabel = (newText: string, index: number) => { - if (isSavingEdits) { - return; + const reservedKeys = [ + ...allExistingKeys.filter((key) => !labels.includes(key)), + ...unsavedLabels, + ]; + + const validateLabel = (text: string): string | null => { + console.log('Validating label:', text, 'Reserved keys:', reservedKeys); + if (reservedKeys.includes(text)) { + const error = `"${text}" already exists. Use a unique name that doesn't match any existing key or property`; + console.log('Validation error:', error); + return error; + } else if (text.length > 63) { + return "Label text can't exceed 63 characters"; } + return null; + }; + + const editUnsavedLabel = (newText: string, index: number) => { + if (isSavingEdits) return; const copy = [...unsavedLabels]; + const oldText = copy[index]; copy[index] = newText; setUnsavedLabels(copy); + + const error = validateLabel(newText); + const newErrors = { ...labelErrors }; + delete newErrors[oldText]; + if (error) { + newErrors[newText] = error; + } + setLabelErrors(newErrors); }; + const removeUnsavedLabel = (text: string) => { - if (isSavingEdits) { - return; - } + if (isSavingEdits) return; setUnsavedLabels(unsavedLabels.filter((label) => label !== text)); }; + const addUnsavedLabel = (text: string) => { - if (isSavingEdits) { + if (isSavingEdits) return; + if (!text.trim()) return; + + const error = validateLabel(text.trim()); + if (error) { + setLabelErrors(prev => ({ ...prev, [text.trim()]: error })); return; } - setUnsavedLabels([...unsavedLabels, text]); + + setUnsavedLabels(prev => [...prev, text.trim()]); + const newErrors = { ...labelErrors }; + delete newErrors[text.trim()]; + setLabelErrors(newErrors); }; - // Don't allow a label that matches a non-label property key or another label (as they stand before saving) - // Note that this means if you remove a label and add it back before saving, that is valid - const reservedKeys = [ - ...allExistingKeys.filter((key) => !labels.includes(key)), - ...unsavedLabels, - ]; - - const [isAddLabelModalOpen, setIsAddLabelModalOpen] = React.useState(false); - const [addLabelInputValue, setAddLabelInputValue] = React.useState(''); - const addLabelInputRef = React.useRef(null); let addLabelValidationError: string | null = null; if (reservedKeys.includes(addLabelInputValue)) { addLabelValidationError = 'Label must not match an existing label or property key'; @@ -75,22 +92,22 @@ const EditableLabelsDescriptionListGroup: React.FC { - setAddLabelInputValue(''); - setIsAddLabelModalOpen(!isAddLabelModalOpen); - }; - React.useEffect(() => { - if (isAddLabelModalOpen && addLabelInputRef.current) { - addLabelInputRef.current.focus(); - } - }, [isAddLabelModalOpen]); - - const addLabelModalSubmitDisabled = !addLabelInputValue || !!addLabelValidationError; - const submitAddLabelModal = (event?: React.FormEvent) => { - event?.preventDefault(); - if (!addLabelModalSubmitDisabled) { - addUnsavedLabel(addLabelInputValue); - toggleAddLabelModal(); + const handleEditComplete = (_event: any, newText: string, index: number) => { + console.log('Edit complete:', newText, 'Index:', index); + const error = validateLabel(newText); + if (error) { + console.log('Setting error for:', newText, error); + setLabelErrors(prev => ({ ...prev, [newText]: error })); + } else { + const newLabels = [...unsavedLabels]; + const oldLabel = newLabels[index]; + newLabels[index] = newText; + setUnsavedLabels(newLabels); + // Clear any previous errors for both old and new labels + const newErrors = { ...labelErrors }; + delete newErrors[oldLabel]; + delete newErrors[newText]; + setLabelErrors(newErrors); } }; @@ -103,57 +120,79 @@ const EditableLabelsDescriptionListGroup: React.FC 0} contentWhenEditing={ - + { + console.log('Adding new label:', newText); + const error = validateLabel(newText); + if (error) { + setLabelErrors(prev => ({ ...prev, [newText]: error })); + } else { + setUnsavedLabels(prev => [...prev, newText]); + const newErrors = { ...labelErrors }; + delete newErrors[newText]; + setLabelErrors(newErrors); + } + }} + > + Add label + + ) + } + > + {unsavedLabels.map((label, index) => ( - ) - } - > - {unsavedLabels.map((label, index) => ( - - ))} - + ))} + + {Object.keys(labelErrors).length > 0 && ( + + )} + } onEditClick={() => { setUnsavedLabels(labels); + setLabelErrors({}); setIsEditing(true); }} onSaveEditsClick={async () => { + if (Object.keys(labelErrors).length > 0) return; setIsSavingEdits(true); try { - await saveEditedLabels(unsavedLabels); + await onLabelsChange(unsavedLabels); } finally { setIsSavingEdits(false); + setIsEditing(false); } - setIsEditing(false); }} onDiscardEditsClick={() => { setUnsavedLabels(labels); @@ -168,56 +207,6 @@ const EditableLabelsDescriptionListGroup: React.FC - {isAddLabelModalOpen ? ( - - Save - , - , - ]} - > -
- - , value: string) => - setAddLabelInputValue(value) - } - ref={addLabelInputRef} - isRequired - validated={addLabelValidationError ? 'error' : 'default'} - /> - {addLabelValidationError && ( - - - } variant="error"> - {addLabelValidationError} - - - - )} - -
- - ) : null} ); }; - -export default EditableLabelsDescriptionListGroup; diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx index 7bf00b05e0..81d0a8dfcd 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx @@ -3,7 +3,7 @@ import { DescriptionList, Flex, FlexItem, TextVariants, Title } from '@patternfl import { ModelVersion } from '~/concepts/modelRegistry/types'; import DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; import EditableTextDescriptionListGroup from '~/components/EditableTextDescriptionListGroup'; -import EditableLabelsDescriptionListGroup from '~/components/EditableLabelsDescriptionListGroup'; +import {EditableLabelsDescriptionListGroup} from '~/components/EditableLabelsDescriptionListGroup'; import ModelPropertiesDescriptionListGroup from '~/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup'; import { getLabels, mergeUpdatedLabels } from '~/pages/modelRegistry/screens/utils'; import useModelArtifactsByVersionId from '~/concepts/modelRegistry/apiHooks/useModelArtifactsByVersionId'; @@ -61,7 +61,7 @@ const ModelVersionDetailsView: React.FC = ({ labels={getLabels(mv.customProperties)} isArchive={isArchiveVersion} allExistingKeys={Object.keys(mv.customProperties)} - saveEditedLabels={(editedLabels) => + onLabelsChange={(editedLabels) => apiState.api .patchModelVersion( {}, diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx index f38366f0a1..6db09bb520 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx @@ -4,7 +4,7 @@ import { RegisteredModel } from '~/concepts/modelRegistry/types'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; import DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; import EditableTextDescriptionListGroup from '~/components/EditableTextDescriptionListGroup'; -import EditableLabelsDescriptionListGroup from '~/components/EditableLabelsDescriptionListGroup'; +import { EditableLabelsDescriptionListGroup } from '~/components/EditableLabelsDescriptionListGroup'; import ModelTimestamp from '~/pages/modelRegistry/screens/components/ModelTimestamp'; import { getLabels, mergeUpdatedLabels } from '~/pages/modelRegistry/screens/utils'; import ModelPropertiesDescriptionListGroup from '~/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup'; @@ -51,7 +51,7 @@ const ModelDetailsView: React.FC = ({ labels={getLabels(rm.customProperties)} isArchive={isArchiveModel} allExistingKeys={Object.keys(rm.customProperties)} - saveEditedLabels={(editedLabels) => + onLabelsChange={(editedLabels) => apiState.api .patchRegisteredModel( {}, From 62c8032c9312888b933be716608c80136badf3d0 Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Fri, 1 Nov 2024 11:12:03 -0400 Subject: [PATCH 02/15] saving --- .../EditableLabelsDescriptionListGroup.tsx | 253 ++++++++---------- 1 file changed, 106 insertions(+), 147 deletions(-) diff --git a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx index 970f74bd5d..80afe34b37 100644 --- a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx +++ b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx @@ -27,8 +27,6 @@ export const EditableLabelsDescriptionListGroup: React.FC = const [isEditing, setIsEditing] = useState(false); const [isSavingEdits, setIsSavingEdits] = useState(false); const [unsavedLabels, setUnsavedLabels] = useState(labels); - const [addLabelInputValue, setAddLabelInputValue] = useState(''); - const [isAddLabelModalOpen, setIsAddLabelModalOpen] = useState(false); const [labelErrors, setLabelErrors] = useState<{ [key: string]: string }>({}); const reservedKeys = [ @@ -37,10 +35,8 @@ export const EditableLabelsDescriptionListGroup: React.FC = ]; const validateLabel = (text: string): string | null => { - console.log('Validating label:', text, 'Reserved keys:', reservedKeys); if (reservedKeys.includes(text)) { const error = `"${text}" already exists. Use a unique name that doesn't match any existing key or property`; - console.log('Validation error:', error); return error; } else if (text.length > 63) { return "Label text can't exceed 63 characters"; @@ -48,165 +44,128 @@ export const EditableLabelsDescriptionListGroup: React.FC = return null; }; - const editUnsavedLabel = (newText: string, index: number) => { - if (isSavingEdits) return; - const copy = [...unsavedLabels]; - const oldText = copy[index]; - copy[index] = newText; - setUnsavedLabels(copy); - - const error = validateLabel(newText); - const newErrors = { ...labelErrors }; - delete newErrors[oldText]; - if (error) { - newErrors[newText] = error; - } - setLabelErrors(newErrors); - }; - const removeUnsavedLabel = (text: string) => { if (isSavingEdits) return; setUnsavedLabels(unsavedLabels.filter((label) => label !== text)); }; - const addUnsavedLabel = (text: string) => { - if (isSavingEdits) return; - if (!text.trim()) return; - - const error = validateLabel(text.trim()); - if (error) { - setLabelErrors(prev => ({ ...prev, [text.trim()]: error })); - return; - } - - setUnsavedLabels(prev => [...prev, text.trim()]); - const newErrors = { ...labelErrors }; - delete newErrors[text.trim()]; - setLabelErrors(newErrors); - }; - - let addLabelValidationError: string | null = null; - if (reservedKeys.includes(addLabelInputValue)) { - addLabelValidationError = 'Label must not match an existing label or property key'; - } else if (addLabelInputValue.length > 63) { - addLabelValidationError = "Label text can't exceed 63 characters"; - } - - const handleEditComplete = (_event: any, newText: string, index: number) => { - console.log('Edit complete:', newText, 'Index:', index); + const handleEditComplete = (_event: any, newText: string, _index: number) => { const error = validateLabel(newText); if (error) { - console.log('Setting error for:', newText, error); setLabelErrors(prev => ({ ...prev, [newText]: error })); } else { - const newLabels = [...unsavedLabels]; - const oldLabel = newLabels[index]; - newLabels[index] = newText; - setUnsavedLabels(newLabels); - // Clear any previous errors for both old and new labels - const newErrors = { ...labelErrors }; - delete newErrors[oldLabel]; - delete newErrors[newText]; - setLabelErrors(newErrors); + setUnsavedLabels(prev => [...prev, newText]); + setLabelErrors({}); } }; return ( - <> - 0} - contentWhenEditing={ - <> - { - console.log('Adding new label:', newText); - const error = validateLabel(newText); - if (error) { - setLabelErrors(prev => ({ ...prev, [newText]: error })); - } else { - setUnsavedLabels(prev => [...prev, newText]); - const newErrors = { ...labelErrors }; - delete newErrors[newText]; - setLabelErrors(newErrors); - } - }} - > - Add label - - ) - } - > - {unsavedLabels.map((label, index) => ( + 0} + contentWhenEditing={ + <> + removeUnsavedLabel(label)} - closeBtnProps={{ isDisabled: isSavingEdits }} - onEditComplete={(_event, newText) => handleEditComplete(_event, newText, index)} + data-testid="add-label-button" + color="blue" + variant="outline" + isEditable + editableProps={{ + 'aria-label': 'Add label', + defaultValue: '', + 'data-testid': 'add-label-input' + }} + onEditComplete={(_event, newText) => { + const error = validateLabel(newText); + if (error) { + setLabelErrors(prev => ({ ...prev, [newText]: error })); + } else { + setUnsavedLabels(prev => [...prev, newText]); + const newErrors = { ...labelErrors }; + delete newErrors[newText]; + setLabelErrors(newErrors); + } + }} > - {label} + Add label - ))} - - {Object.keys(labelErrors).length > 0 && ( - - )} - - } - onEditClick={() => { - setUnsavedLabels(labels); - setLabelErrors({}); - setIsEditing(true); - }} - onSaveEditsClick={async () => { - if (Object.keys(labelErrors).length > 0) return; - setIsSavingEdits(true); - try { - await onLabelsChange(unsavedLabels); - } finally { - setIsSavingEdits(false); - setIsEditing(false); - } - }} - onDiscardEditsClick={() => { - setUnsavedLabels(labels); + ) + } + > + {unsavedLabels.map((label, index) => ( + + ))} + + {Object.keys(labelErrors).length > 0 && ( + + )} + + } + onEditClick={() => { + setUnsavedLabels(labels); + setLabelErrors({}); + setIsEditing(true); + }} + onSaveEditsClick={async () => { + if (Object.keys(labelErrors).length > 0) return; + setIsSavingEdits(true); + try { + await onLabelsChange(unsavedLabels); + } finally { + setIsSavingEdits(false); setIsEditing(false); - }} - > - - {labels.map((label) => ( - - ))} - - - + } + }} + onDiscardEditsClick={() => { + setUnsavedLabels(labels); + setIsEditing(false); + }} + > + + {labels.map((label) => ( + + ))} + + ); }; From 14ff57d110478647eacea4440fae1614d3e48a3b Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Mon, 4 Nov 2024 10:45:37 -0500 Subject: [PATCH 03/15] lint errors fix --- .../DashboardDescriptionListGroup.tsx | 3 +- .../EditableLabelsDescriptionListGroup.tsx | 43 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/frontend/src/components/DashboardDescriptionListGroup.tsx b/frontend/src/components/DashboardDescriptionListGroup.tsx index e294be826d..110f629c83 100644 --- a/frontend/src/components/DashboardDescriptionListGroup.tsx +++ b/frontend/src/components/DashboardDescriptionListGroup.tsx @@ -58,6 +58,7 @@ const DashboardDescriptionListGroup: React.FC @@ -75,7 +76,7 @@ const DashboardDescriptionListGroup: React.FC diff --git a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx index 80afe34b37..a5fad2bfad 100644 --- a/frontend/src/components/EditableLabelsDescriptionListGroup.tsx +++ b/frontend/src/components/EditableLabelsDescriptionListGroup.tsx @@ -1,10 +1,5 @@ import React, { useState } from 'react'; -import { - Label, - LabelGroup, - Alert, - AlertVariant -} from '@patternfly/react-core'; +import { Label, LabelGroup, Alert, AlertVariant } from '@patternfly/react-core'; import DashboardDescriptionListGroup from './DashboardDescriptionListGroup'; interface EditableLabelsProps { @@ -36,25 +31,27 @@ export const EditableLabelsDescriptionListGroup: React.FC = const validateLabel = (text: string): string | null => { if (reservedKeys.includes(text)) { - const error = `"${text}" already exists. Use a unique name that doesn't match any existing key or property`; - return error; - } else if (text.length > 63) { + return `"${text}" already exists. Use a unique name that doesn't match any existing key or property`; + } + if (text.length > 63) { return "Label text can't exceed 63 characters"; } return null; }; const removeUnsavedLabel = (text: string) => { - if (isSavingEdits) return; + if (isSavingEdits) { + return; + } setUnsavedLabels(unsavedLabels.filter((label) => label !== text)); }; - const handleEditComplete = (_event: any, newText: string, _index: number) => { + const handleEditComplete = (_event: MouseEvent | KeyboardEvent, newText: string) => { const error = validateLabel(newText); if (error) { - setLabelErrors(prev => ({ ...prev, [newText]: error })); + setLabelErrors((prev) => ({ ...prev, [newText]: error })); } else { - setUnsavedLabels(prev => [...prev, newText]); + setUnsavedLabels((prev) => [...prev, newText]); setLabelErrors({}); } }; @@ -87,14 +84,14 @@ export const EditableLabelsDescriptionListGroup: React.FC = editableProps={{ 'aria-label': 'Add label', defaultValue: '', - 'data-testid': 'add-label-input' + 'data-testid': 'add-label-input', }} onEditComplete={(_event, newText) => { const error = validateLabel(newText); if (error) { - setLabelErrors(prev => ({ ...prev, [newText]: error })); + setLabelErrors((prev) => ({ ...prev, [newText]: error })); } else { - setUnsavedLabels(prev => [...prev, newText]); + setUnsavedLabels((prev) => [...prev, newText]); const newErrors = { ...labelErrors }; delete newErrors[newText]; setLabelErrors(newErrors); @@ -106,22 +103,22 @@ export const EditableLabelsDescriptionListGroup: React.FC = ) } > - {unsavedLabels.map((label, index) => ( + {unsavedLabels.map((label) => (