-
Notifications
You must be signed in to change notification settings - Fork 10
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: Support drag file to open #33
Conversation
This pull request is deployed automatically with Vercel. samuwrite |
src/app/app.tsx
Outdated
const dropRef = useFileDrop({ file }); | ||
|
||
return ( | ||
<div className={s.app}> | ||
<div className={s.app} ref={dropRef}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, but know that there's an alternative where we should define the ref outside and pass it as param. In those cases, it would be simpler if an element needs to have several refs (not this case though)
src/app/state/file-drop.ts
Outdated
export const useFileDrop = (params: Params): React.RefObject<HTMLDivElement> => { | ||
const dropRef = useRef<HTMLDivElement>(null); | ||
|
||
const handleDragOver = (e: DragEvent) => e.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be defined outside of the hook. Would be a little bit better if we don't re-create it unnecessarily
src/app/state/file-drop.ts
Outdated
|
||
useEffect(() => { | ||
const { current } = dropRef; | ||
if (!current) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should throw Error instead of simply return. If we forget to attach the ref, the drag and drop would not working, which should be an error in user's perpective
src/app/state/file-drop.ts
Outdated
const { items } = e.dataTransfer; | ||
if (1 < items.length) throw Error("Only one file can be upload at a time"); | ||
if (Array.from(items).some((item) => item.kind !== "file")) | ||
throw Error("Support upload single file only"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message and the one at line 16 sound very similar. One should be about the number of files, while the other is about the "folder" type not supported
src/app/state/file-drop.ts
Outdated
if (1 < items.length) throw Error("Only one file can be upload at a time"); | ||
if (Array.from(items).some((item) => item.kind !== "file")) | ||
throw Error("Support upload single file only"); | ||
if (items && items.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm why do we still need these checks? Shouldn't it be checked already above?
src/app/state/file-drop.ts
Outdated
params.file.setHandle(file as FileSystemFileHandle); | ||
params.file.setDirty(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to see this is quite repetitive, and it's easy to forget. I think there should be a method in the FileState that simply calls these 2:
params.file.setFile(file); // set handle AND set dirty to false
src/app/state/file-drop.ts
Outdated
} catch (e) { | ||
throw Error(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only catch to throw then we don't need the try at all. The error would still be throw up if there is no catch
src/app/state/file-drop.ts
Outdated
current.removeEventListener("dragover", handleDragOver); | ||
current.removeEventListener("drop", handleDrop); | ||
}; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why there is no dependency array!
Resolve #28