feat(ws): Add properties tile to new workspace creation #262
Conversation
Signed-off-by: Hen Schwartz (EXT-Nokia) <[email protected]>
|
[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) <[email protected]>
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.
There was a problem hiding this comment.
Hi Paulo, do you mean to take the read-only button to the create modal, put it inside, and disable the availability to edit it from the actual table? it means that it will be available for edit inside the next modal of edit exiting row in the table...
please confirm if that what you meant for
There was a problem hiding this comment.
@henschwartz yes that is right, it would be only for visualization at the table.
Maybe @jenny-s51 could give an input on that to confirm it is better this way.
There was a problem hiding this comment.
Great call-out @paulovmr - yes, for consistency we should handle the "read only" configuration in the modal and display the value in the table, rather than the switch. The "Edit" functionality should be reserved for within the modal.
|
/ok-to-test |
| <Switch | ||
| id={`readonly-switch-${index}`} | ||
| isChecked={volume.readOnly} | ||
| onChange={() => { | ||
| const updated = [...volumes]; | ||
| updated[index].readOnly = !updated[index].readOnly; | ||
| setVolumes(updated); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
| <Switch | |
| id={`readonly-switch-${index}`} | |
| isChecked={volume.readOnly} | |
| onChange={() => { | |
| const updated = [...volumes]; | |
| updated[index].readOnly = !updated[index].readOnly; | |
| setVolumes(updated); | |
| }} | |
| /> | |
| {volume.readOnly ? 'Enabled' : 'Disabled'} |
| <Tr> | ||
| <Th>PVC Name</Th> | ||
| <Th>Mount Path</Th> | ||
| <Th>Read Only</Th> |
There was a problem hiding this comment.
| <Th>Read Only</Th> | |
| <Th>Read-only Access</Th> |
| onChange={(_, val) => setFormData({ ...formData, mountPath: val })} | ||
| id="mount-path" | ||
| /> | ||
| </FormGroup> |
There was a problem hiding this comment.
| </FormGroup> | |
| </FormGroup> | |
| <FormGroup | |
| fieldId="readonly-access" | |
| > | |
| <Switch | |
| id="readonly-access-switch" | |
| label="Enable read-only access" | |
| isChecked={formData.readOnly} | |
| onChange={() => setFormData({ ...formData, readOnly: !formData.readOnly })} | |
| /> | |
| </FormGroup> |
|
Thanks for making these updates @henschwartz! I've left a few specific suggestions directly on the code to implement these changes following the thread above (cc @paulovmr). The main points are:
This approach should provide a more intuitive flow for users by keeping all settings together in the modal. Updated table: Just noting that with these changes, there is a vertical spacing difference between the text inputs and the switch in the modal. This is related to the text inputs not using |
Signed-off-by: Hen Schwartz (EXT-Nokia) <[email protected]>
| } from '@patternfly/react-core'; | ||
| import { WorkspaceVolume } from '~/shared/types'; | ||
|
|
||
| interface WorkspaceCreationPropertiesVolumes { |
There was a problem hiding this comment.
| interface WorkspaceCreationPropertiesVolumes { | |
| interface WorkspaceCreationPropertiesVolumesProps { |
| setVolumes: React.Dispatch<React.SetStateAction<WorkspaceVolume[]>>; | ||
| } | ||
|
|
||
| export const WorkspaceCreationPropertiesVolumesProps: React.FC< |
There was a problem hiding this comment.
| export const WorkspaceCreationPropertiesVolumesProps: React.FC< | |
| export const WorkspaceCreationPropertiesVolumes: React.FC< |
| } | ||
|
|
||
| export const WorkspaceCreationPropertiesVolumesProps: React.FC< | ||
| WorkspaceCreationPropertiesVolumes |
There was a problem hiding this comment.
| WorkspaceCreationPropertiesVolumes | |
| WorkspaceCreationPropertiesVolumesProps |
Signed-off-by: Hen Schwartz (EXT-Nokia) <[email protected]>
this PR resolve issue #225