feat: added attachment field #428
Conversation
| field: Field; | ||
| } | ||
|
|
||
| async function fetchCsrfToken() { |
There was a problem hiding this comment.
Maybe we could turned this into a useCsrfToken reusable hook and memoize it so we only get it once independently of where it's used.
There was a problem hiding this comment.
I am not sure how the CSRF token works. Does it expire? For example, if the user opens a page and fetches the CSRF token, then it stays idle for 1 hour, and then he starts an upload, is the token still valid?
If the answer is yes, we can use a hook and memoize it. Otherwise, it is better to always fetch it before using it.
There was a problem hiding this comment.
It is based on the session so in theory in your example it should still be valid as long as the session is alive.
Otherwise, it is better to always fetch it before using it.
My only concern is if we need several times, e.g. upload and form submission, it may lead to unnecessary calls.
Totally fine to address it later on as well 👍🏼
There was a problem hiding this comment.
If we can safely fetch it once it is better to memoize it to avoid unnecessary calls, but let's revisit it later when we'll need to use it in another places
src/modules/new-request-form/fields/attachments/Attachments.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const { addToast } = useToast(); | ||
|
|
||
| const notifyError = useCallback( |
There was a problem hiding this comment.
Do we need the useCallback in this case?
There was a problem hiding this comment.
It seems that the onDrop from react-dropzone needs the useCallback: https://react-dropzone.js.org/. Our onDrop is calling notifyError so it needs to be passed in the dependency array of it and memorized as well.
src/modules/new-request-form/fields/attachments/FileListItem.tsx
Outdated
Show resolved
Hide resolved
| : file | ||
| ) | ||
| ); | ||
| }, []); |
There was a problem hiding this comment.
Do we need to memoize all of these?
There was a problem hiding this comment.
As above, all these functions are used by the onDrop callback and they need to be memorized as well
src/modules/new-request-form/fields/attachments/AttachedFile.ts
Outdated
Show resolved
Hide resolved
Fredx87
force-pushed
the
gianluca/attachments
branch
from
c5e6bf9
to
beb1a88
Compare
Description
This PR adds the management of attachments, using public APIs.
I tried to replicate the existing logic as closely as possible, I just changed a bit the error handling. In the existing implementation, there is a logic for displaying different error messages based on the response returned by the API. I tried to replicate the error cases, for example uploading a big file, but I wasn't able to trigger an error from the API, even with a fairly big attachment. I decided to implement a simple error handling for now, when we get an error either from the API or the client side upload, we just show a generic error message:
There was an error uploading {fileName}. Please try again or upload another file.. We can eventually revisit in the future if we need more specific errors, but as I said it is not clear what are the possible failures.Looking at the current implementation, it seems that the eventual error is displayed inline near the attachments. I decided instead to use a Garden notification
It seems that there was also a logic for preventing the form submission while an upload was in progress, but I wasn't able to replicate this behavior, so for simplicity, the form is always submitted even if there are some uploads in progress. In this case, the pending uploads are simply not saved on the ticket.
Screenshots
attachments-demo.mov
Checklist