Issue 42 #57
Issue 42 #57
Conversation
There was a problem hiding this comment.
Thank you @ariannmichael!
However, there is several points to improve before accepting this PR. Possibly extract some components, remove the use of overly specified modals, API calls that do not throw which breaks react-query and correct how state is updated.
| setTensors(newTensors) | ||
| } | ||
|
|
||
| const toBase64 = file => |
There was a problem hiding this comment.
POST /dcfl/datasets accepts a file or a (plain) string but not base64 — @IonesioJunior can you confirm?
|
|
||
| const renderTensorInputs = () => { | ||
| return tensors.map((el, i) => ( | ||
| <div key={i}> |
| return tensors.map((el, i) => ( | ||
| <div key={i}> | ||
| <div> | ||
| <label htmlFor="tensor_name" className="text-sm block font-bold pb-2"> |
There was a problem hiding this comment.
Apply DRY here - inputs can be components or use an array to denote the form inputs and map over it. This becomes a lot of code real fast.
| const {name, value} = event.target | ||
|
|
||
| const newFormObject = formObject | ||
| newFormObject[name] = value |
There was a problem hiding this comment.
Do not mutate state directly.
React will not go through the motions as expected if you update the state directly. It does strict equality comparison and you're changing a property but not the object itself — a re-render is not triggered.
| } | ||
|
|
||
| const EditDatasetModal: FunctionComponent<IEditDatasetModalProps> = ({isOpen, onClose, onConfirm, dataset}) => { | ||
| const [formObject, setFormObject] = useState({ |
There was a problem hiding this comment.
Hm, I think useReducer might make more sense here.
However, this is not needed at all — use react-hook-form to handle the form instead.
| <strong>Schema</strong>:{' '} | ||
| <span className="text-gray-400"> | ||
| <ul className="list-horizontal"> | ||
| {tensor.manifest.split(',').map(el => ( |
| <strong>File</strong>: <FileIcon className="h-4 text-gray-400" /> <span>{name}</span> | ||
| </span> | ||
| <span> | ||
| <strong>Shape</strong>: <span className="text-gray-400">{JSON.stringify(tensor.shape)}</span> |
| <span className="text-gray-400"> | ||
| <ul className="list-horizontal"> | ||
| {tensor.manifest.split(',').map(el => ( | ||
| <li key={`${el}-element`}>{el}</li> |
There was a problem hiding this comment.
This causes problems as el is potentially not unique in tensor.manifest.split(',')
| const router = useRouter() | ||
| const {slug} = router.query | ||
|
|
||
| const [openDeleteDatasetModal, setOpenDeleteDatasetModal] = useState(false) |
| @@ -24,6 +27,18 @@ export interface ICreateGroupResponse { | |||
| message: string | |||
| } | |||
|
|
|||
| export interface ICreateDatasetResponse { | |||
There was a problem hiding this comment.
Several identical types. Is it possible to generalize?
Description
Closes #42
Checklist