Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(adminPanel): RN-1248: Drag & drop file upload #5713

Open
wants to merge 46 commits into
base: dev
Choose a base branch
from

Conversation

jaskfla
Copy link
Contributor

@jaskfla jaskfla commented Jun 7, 2024

Issue RN-1248: Add drag and drop functionality for file upload

Changes


Screenshots

Note

More to come! But pending a few (very minor) CSS tweaks

Screenshot 2024-06-14T163155

Screenshot 2024-06-14T163141

Works on DataTrak too

Screenshot 2024-06-14T163730

@jaskfla jaskfla changed the title feat(adminPanel): RR-1248: Dra & drop file upload feat(adminPanel): RR-1248: Drag & drop file upload Jun 7, 2024
@jaskfla jaskfla changed the title feat(adminPanel): RR-1248: Drag & drop file upload feat(adminPanel): RN-1248: Drag & drop file upload Jun 7, 2024
yarn.lock Outdated Show resolved Hide resolved
@jaskfla jaskfla marked this pull request as ready for review June 14, 2024 04:43
textOnButton,
showFileSize,
maxSizeInBytes,
initialFileName = null,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this wasn’t actually doing anything. Even in EditSurveyPage (the only usage that provides an initialFileName), it wasn’t actually displayed. The FileUploadField just always accurately reflected what files were(n’t) selected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out DataTrak might use this (trusted my editor too much 🥲). I’ll figure out what to do about this

Copy link
Contributor

@alexd-bes alexd-bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one question in there, but otherwise happy for it to go to testing

);
setFiles(files.concat(deduped));
console.log(files);
onChange(files, event as React.ChangeEvent<HTMLInputElement>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, is there any particular reason to manage files in state AND from the parent? Do you foresee any bugs managing both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m irked by it, to be honest. The only reason this component manages state is so that files can be removed. (My approach doesn’t use inputRef.current.value.)

I started off with state management only in the parent (i.e. existing behaviour), and just passed the acceptedFiles up to the parent, but the react-dropzone API has no way to remove or clear files (except by making a new file selection). Had a word with Felicity and her preference is to be able to add/remove files individually. Rationale being that <input>’s normal behaviour of having to do multi-selects “is one go” isn’t super obvious to people with lower technical self-efficacy

Not so worried about bugs, but I do think this is awkward to trace/debug. If you have ideas I’d love to hear 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's annoying. So doing onChange with a filtered array of files won't remove the file from the list?

If that doesn't work, you could also just do a useEffect on change of files array (probably of stringified file names array or something, so it doesn't re-render too many times) and call onChange there so that you don't have to remember to add it to he various on change handlers inside this component. Also adding some comments, so someone (me) doesn't inadvertently break it later 😅

Copy link
Contributor Author

@jaskfla jaskfla Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the trouble is you can add and remove files individually. So when multiple=true, you don’t have to ⌘-click or ⇧-click to select multiple files all at once. You can drop some files, then drop some more files, which is why the setFiles call concatenates the new selection to the existing selection. It doesn’t just replace it

The difficulty with using only file name (and the reason which I stringify the entire File object) is that there is an edge case where you might select two files with the same name from different folders. The browser doesn’t reveal a fully qualified path to us. We just get the basename, so it isn’t actually guaranteed to uniquely identify a file 😵

In fact, even JSON.stringify(file) isn’t guaranteed to work perfectly, but I just figured it’s extremely unlikely for two non-identical files to have the same name, filesize and last-modified date

useEffect seems like a good idea regardless—I’ll give that a go! (Will also add comments—thanks for the bump)

Copy link
Contributor Author

@jaskfla jaskfla Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up deciding against useEffect because:

  1. setFiles is called in only one place
  2. the way React queues updates could cause it to run later than expected

Using the change handler is how the React docs advise notifying the parent about state changes

Turns out I do use it in two places. I’ve gone against React docs’ advice and just used useEffect

merge: update with latest dev before testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants