Conversation
There was a problem hiding this comment.
Hey everyone. Great to see how much progress you have made already!
I didn't point out the same thing multiple times even if I saw it multiple times. Please try to find other occurrences of the same thing when you see my comments.
Here are a couple of things that I saw a lot:
- Some exports are missing resulting in imports with lots of depth.
- Sometimes you don't use
useCallbackenough. (Search for={() =>across the codebase, but also look for functions that you just create within components.) - You should use translation data instead of replacing underscores manually.
- Adding more dedicated props files would make the code a lot cleaner.
- You still have a couple // TODO and // Fix comments that you might want to take a look at before merging.
- You should use
useQueryinstead of callbacks that fetch something which are called fromuseEffect(you mostly do this already).
apps/editor/src/components/data-manager/project-view-switch/project-view-switch.tsx
Outdated
Show resolved
Hide resolved
apps/editor/src/components/data-manager/project-views/jobs-view.tsx
Outdated
Show resolved
Hide resolved
| const loadImagesAndAnnotations = () => { | ||
| async function asyncfunc() { | ||
| if (store?.editor.activeDocument?.layers.length !== 0) { | ||
| return store?.destroyReload(); | ||
| } | ||
| const fileTransfer = new DataTransfer(); | ||
| const imageIdToOpen = searchParams.get("imageId"); | ||
| if (imageIdToOpen) { | ||
| try { | ||
| const imageFile = await fetchImageFile(imageIdToOpen); | ||
| fileTransfer.items.add(imageFile); | ||
| } catch (error) { | ||
| store?.setError({ | ||
| titleTx: "import-error", | ||
| descriptionTx: "image-open-error", | ||
| }); | ||
| } | ||
| } | ||
| const annotationIdToOpen = searchParams.get("annotationId"); | ||
| if (annotationIdToOpen) { | ||
| try { | ||
| const annotationFile = await fetchAnnotationFile( | ||
| annotationIdToOpen, | ||
| ); | ||
| fileTransfer.items.add(annotationFile); | ||
| } catch (error) { | ||
| store?.setError({ | ||
| titleTx: "import-error", | ||
| descriptionTx: "annotation-open-error", | ||
| }); | ||
| } | ||
| } | ||
| if (store && fileTransfer.files.length) { | ||
| importFilesToDocument(fileTransfer.files, store); | ||
| } | ||
| } | ||
| asyncfunc(); | ||
| }; | ||
| useEffect(loadImagesAndAnnotations, [searchParams, store]); |
There was a problem hiding this comment.
This pattern of creating a callback that fetches something and then calling it form a useEffect is quite dirty. Instead you can use useQuery.
PaulBrachmann
left a comment
PaulBrachmann
left a comment
There was a problem hiding this comment.
Thank you @jonaskordt for doing the heavy lifting in terms of the review already!
I can only agree, it's quite impressive what you came up with and how well you integrated your extensions into the existing codebase.
I added a few more comments. -- And while we're at those, please make sure you use // Sentence case in your comments consistently (if you don't have a good reason not to).
Keep up the great work!
apps/editor/src/components/data-manager/dataset-explorer/dataset-explorer.tsx
Outdated
Show resolved
Hide resolved
apps/editor/src/components/data-manager/dataset-explorer/dataset-explorer.tsx
Outdated
Show resolved
Hide resolved
apps/editor/src/components/data-manager/dataset-explorer/dataset-explorer.tsx
Outdated
Show resolved
Hide resolved
| import { Job } from "../types"; | ||
| import { hubBaseUrl } from "./hub-base-url"; | ||
|
|
||
| const getJobsBy = async (projectId: string) => { |
There was a problem hiding this comment.
I'd probably call these getJobsByProject, etc.
| return { | ||
| isDeleteProjectsError: isError, | ||
| isDeleteProjectsIdle: isIdle, | ||
| isDeleteProjectsLoading: isLoading, | ||
| isDeleteProjectsPaused: isPaused, | ||
| isDeleteProjectsSuccess: isSuccess, | ||
| deleteProjects: mutate, | ||
| }; |
There was a problem hiding this comment.
Why do you invent your own interface here?
I think it might be cleaner to stick to the one provided by react-query and use your hooks like this:
const deleteProjects = useDeleteProjectsMutation();
...
deleteProjects.mutate();
|
Hi Jonas and Paul, thanks a lot that you took the time to review this gigantic PR! The comments are very helpful and we will try to implement them within the next week. |
Duplicate Layer option
…nto layer-group-ui
Refactor metadata
feat: add dragndrop layergroup ui
Image list refactor
Fix saving issue
Layer group dnd for strategies
Red big dockey
fix: layer group ui bug with incorrect preview
9 participants
closes #400
First release for Annotation Service.
We know the Pull Request is packed with features, but we value your feedback and would appreciate any input.
Glhf :)