Added resetOnChange property and custom handler #27
Conversation
src/components/FileUploadButton.tsx
Outdated
| inputRef.current.blur(); | ||
| } | ||
| }} | ||
| onChange={handleFileChange(handleFiles, resetOnChange)} |
There was a problem hiding this comment.
| onChange={handleFileChange(handleFiles, resetOnChange)} | |
| onChange={() => handleFileChange(handleFiles, resetOnChange)} |
You sure this is correct? You are calling the function on every render ... not on change
There was a problem hiding this comment.
Yes, handleFileChange is just a closure that takes into consideration actual user handler (in this case handleFiles) and resetOnChange property and returns function that handles onChange event. I could memoize it if you think it'd have impact on performance.
There was a problem hiding this comment.
Ahh ok, I missed the part where you return the function from handleFileChange, my bad. Memoization is not necessary. But handleFileChange doesn't exactly handle the file-change-event (which I assumed it would). It only gives you handler. You can consider following change, which avoids calling that function on every render and does exactly what it says:
// utils.js
export const handleFileChange = (
files: WhateverType,
resetOnChange: boolean = false,
handler?: (files: Array<File>) => void, // last since its optional
) => {
if (!files) return;
try {
handler?.(Array.from(files));
} catch (error) {
console.error(error);
}
if (resetOnChange) currentTarget.value = '';
};
// And in FileUploadButton.tsx and ImageUploadButton.tsx
const onFileChange = ({ currentTarget }: React.ChangeEvent<HTMLInputElement>) => {
const updatedFiles = currentTarget.files;
handleFileChange(updatedFiles, resetOnChange, handleFiles);
}
<input
...
onChange={onFileChange}
/>There was a problem hiding this comment.
I see where you're coming from and I'm sorry for confusing function naming. As for your snippet - it makes sense - but I'll go for a hook to make it more React-y and so you wouldn't have to define that function twice.
// utils.ts
export const useHandleFileChangeWrapper = (
resetOnChange: boolean = false,
handler?: (files: Array<File>) => void,
) =>
useCallback(
({ currentTarget }: React.ChangeEvent<HTMLInputElement>) => {
const { files } = currentTarget;
if (!files) return;
try {
handler?.(Array.from(files));
} catch (error) {
console.error(error);
}
if (resetOnChange) currentTarget.value = '';
},
[callback, resetOnChange],
);
// in <File/Image>UploadButton.tsx then:
const onFileChange = useHandleFileChangeWrapper(resetOnChange, handleFiles);
<input onChange={onFileChange} />;This way - no more unwanted renders and naming is clear (at least I think that it's pretty clear). 😄
Right! Not sure about the default value though. Could be possibly a breaking change if set to |
Okay, but wasn't this default behaviour for FileUploadButton before? So defaulting to false would be the breaking change for that component? |
Actualy... that's true. Now what's lesser evil? |
Maybe just set it to true for files and false for images? Although IMO your changes just feel like a bug fix rather than something that really needs to be a separate prop. Not something I feel very strongly about though. |
I'll go for the "true for files and false for images" to preserve default behaviour in case somebody would want to go that way. |
Added
resetOnChangeproperty to both upload buttons so when user deletes file from preview and then decides to upload that same file again, they can. This thing has been already (partially) implemented inFileUploadButtonwith reference toinputelement so I decided to rewrite it a bit and add it toImageUploadButtonas well.Here's the preview of the "bug":
Screen.Recording.2021-04-02.at.2.20.06.PM.mov