(feat) O3-2626: Visit note attach image should be able to attach multiple images #1798
Conversation
jwnasambu
force-pushed
the
(fix)O3-2626b
branch
from
8f51876 to
e517246
Compare
jwnasambu
changed the title
jwnasambu
changed the title
|
@ibacher, @denniskigen and @brandones kindly help me review my PR at your convinient time please! |
brandones
changed the title
|
@jwnasambu Are you stuck on any particular point or is this ready for a full review? |
|
@ibacher Its ready for review. Thanks for asking. |
There was a problem hiding this comment.
Thanks @jwnasambu! Overall functionality looks good, but there are some finishing touches needed here.
| images.map((image) => { | ||
| const uploadedFile: UploadedFile = { | ||
| base64Content: image.base64Content, | ||
| fileName: 'filename', |
There was a problem hiding this comment.
It would be good to either use the image name or allow the user to edit it.
| base64Content: image.base64Content, | ||
| fileName: 'filename', | ||
| fileType: image.fileType, | ||
| fileDescription: 'description', |
There was a problem hiding this comment.
It would be good to collect this from the user somehow or omit it altogether rather than supplying junk data.
| if (filteredFiles) { | ||
| setUploadedImages([...uploadedImages, file]); | ||
| } else { | ||
| console.error('Error: Unsupported file type'); |
There was a problem hiding this comment.
Errors should be displayed as some kind of notification (either an inline or snackbar) and not simply logged to the console.
There was a problem hiding this comment.
Probably want to use reportError for things like this
There was a problem hiding this comment.
In this case, because we want to show an InlineNotification similar to https://github.com/openmrs/openmrs-esm-patient-chart/blob/main/packages/esm-patient-attachments-app/src/camera-media-uploader/media-uploader.component.tsx#L81 here because they're both doing the same thing. So, using an InlineNotification is the ideal approach. We shouldn't log anything to the console.
| @@ -49,7 +49,7 @@ import { | |||
| } from './visit-notes.resource'; | |||
| import styles from './visit-notes-form.scss'; | |||
|
|
|||
| const allowedImageTypes = ['jpeg', 'jpg', 'png', 'webp']; | |||
| const allowedImageTypes = ['.jpeg', '.jpg', '.png', '.webp']; | |||
There was a problem hiding this comment.
This shouldn't be hard-coded. I think we can adopt the approach from the attachments ESM to load the allowed extensions from the global properties.
There was a problem hiding this comment.
@ibacher @NethmiRodrigo's handled that in https://openmrs.atlassian.net/browse/O3-3153. Presently, it's set to return the following allowedFileExtensions: ['jpeg', 'jpg', 'png' , 'pdf'].
Any notable omissions?
There was a problem hiding this comment.
We've had to omit out 'webp' from the list of allowed file extensions since it wasn't a supported form of image as per openmrs-module-attachments (list of supported mime types were found from here)
| @@ -49,7 +49,7 @@ import { | |||
| } from './visit-notes.resource'; | |||
| import styles from './visit-notes-form.scss'; | |||
|
|
|||
| const allowedImageTypes = ['jpeg', 'jpg', 'png', 'webp']; | |||
| const allowedImageTypes = ['.jpeg', '.jpg', '.png', '.webp']; | |||
There was a problem hiding this comment.
| const allowedImageTypes = ['.jpeg', '.jpg', '.png', '.webp']; | |
| const allowedImageTypes = ['jpeg', 'jpg', 'png', 'webp']; |
We shouldn't change these values as they'll break things for other teams. See this comment for context, and this follow-up commit.
There was a problem hiding this comment.
@ibacher kindly I have worked on the proposed changes but I have failed to push and error is displayed. Kindly it has been hours trying to solve the issue without success. Kindly any suggestion on the way forward please?
There was a problem hiding this comment.
Disregard my suggestion above. We should delete that array and replace it with the allowed file extensions specified in the backend.
Basically, you can now get the allowedFileExtensions array from the useAllowedFileExtensions hook that Common Lib exports. To leverage it, rebase your branch against main and then import useAllowedFileExtensions from Common Lib and then get allowedFileExtensions from the hook. You can then replace occurrences of allowedImageTypes with allowedFileExtensions.
Finally, you’ll also want to filter allowedFileExtensions to exclude PDF files as we’re only dealing with images here.
…le images code refactor
|
@ibacher and @denniskigen Kindly am so sorry I have tried to push the changes and even update this PR but all in vain since yesterday and no error thrown to guide me. Kindly I have created this link #1826 with the proposed changes. |
Requirements
Summary
I fixed the VisitNotesForm component to allow attaching multiple images without necessarily being an array by adjusting the UploadedFile type to accommodate multiple files and update the related logic.
Screenshots
Uploading screencast 2024-05-01 5 PM-36-32.mp4…
Related Issue
https://openmrs.atlassian.net/browse/O3-2626
Other