feat(ws): Add properties tile to new workspace creation #262
Conversation
Signed-off-by: Hen Schwartz (EXT-Nokia) <hen.schwartz@nokia.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Great work here @henschwartz! I’ve PR‑ed your branch with a several tweaks, which includes:
|
There was a problem hiding this comment.
Thanks @henschwartz ! I like the updates proposed by @jenny-s51 in her PR to your branch. I left a few more comments below.
| import { WorkspaceCreationPropertiesTable } from '~/app/pages/Workspaces/Creation/properties/WorkspaceCreationPropertiesTable'; | ||
| import { WorkspaceImage } from '~/shared/types'; | ||
|
|
||
| interface Volume { |
There was a problem hiding this comment.
There is a part of the Workspace type that has the volumes inside it. Please consider extracting two types there (WorkspaceVolumes containing what is inside the volumes field and WorkspaceVolume containing what is inside the data field). Then this interface would not be needed, and the WorkspaceVolume type created could be used instead of it.
| FormGroup, | ||
| } from '@patternfly/react-core'; | ||
|
|
||
| interface Volume { |
| readOnly: boolean; | ||
| } | ||
|
|
||
| interface WorkspaceCreationPropertiesTableProps { |
There was a problem hiding this comment.
| interface WorkspaceCreationPropertiesTableProps { | |
| interface WorkspaceCreationPropertiesVolumesProps { |
| setVolumes: React.Dispatch<React.SetStateAction<Volume[]>>; | ||
| } | ||
|
|
||
| export const WorkspaceCreationPropertiesTable: React.FC<WorkspaceCreationPropertiesTableProps> = ({ |
There was a problem hiding this comment.
| export const WorkspaceCreationPropertiesTable: React.FC<WorkspaceCreationPropertiesTableProps> = ({ | |
| export const WorkspaceCreationPropertiesVolumes: React.FC<WorkspaceCreationPropertiesVolumesProps> = ({ |
| const [deleteIndex, setDeleteIndex] = useState<number | null>(null); | ||
| const [dropdownOpen, setDropdownOpen] = useState<number | null>(null); | ||
|
|
||
| const handleAddOrEdit = () => { |
There was a problem hiding this comment.
Please consider using useCallback here.
| resetForm(); | ||
| }; | ||
|
|
||
| const handleEdit = (index: number) => { |
There was a problem hiding this comment.
Please consider using useCallback here.
| setIsModalOpen(true); | ||
| }; | ||
|
|
||
| const openDetachModal = (index: number) => { |
There was a problem hiding this comment.
Please consider using useCallback here.
| setIsDeleteModalOpen(true); | ||
| }; | ||
|
|
||
| const handleDelete = () => { |
There was a problem hiding this comment.
Please consider using useCallback here.
| } | ||
| }; | ||
|
|
||
| const resetForm = () => { |
There was a problem hiding this comment.
Please consider using useCallback here.
Signed-off-by: Hen Schwartz (EXT-Nokia) <hen.schwartz@nokia.com>
There was a problem hiding this comment.
Thanks for the changes @henschwartz ! I left two more comments, and also there are a few linting errors, you can check and automatically fix some of them with npm run test:fix.
| @@ -0,0 +1,205 @@ | |||
| import React, { useCallback, useState } from 'react'; | |||
There was a problem hiding this comment.
This file should be called WorkspaceCreationPropertiesVolumes.tsx.
| /> | ||
| <ModalBody> | ||
| <Form> | ||
| <FormGroup label="PVC Name" isRequired fieldId="pvc-name"> |
There was a problem hiding this comment.
IMO the volume creation modal should allow the user to set the volume as read-only with the slider component, like in the table.
|
/ok-to-test |
this PR resolve issue #225