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.

Sign up for GitHub

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

Jump to bottom

Feat: Support drag file to open #33

Merged
merged 7 commits into from
Sep 2, 2021
Merged

Feat: Support drag file to open #33

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
00e6c80
feat: drop file to open
monodyle Sep 1, 2021
7e51c59
fix: request change issues
monodyle Sep 1, 2021
8498423
wip: dragging state
monodyle Sep 1, 2021
992c4af
Merge branch 'main' into feature/drop-file
thien-do Sep 2, 2021
6cba365
Refactor: Extract AppDrop indicator
thien-do Sep 2, 2021
c8ac6dd
Refactor: Use React for event handler in AppDrop
thien-do Sep 2, 2021
a0c58ee
Chore: Fix linting
thien-do Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/app/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { usePrefs } from "~src/components/prefs/state";
import s from "./app.module.css";
import { useEditorTheme } from "./state/editor-theme";
import { useFileDirty } from "./state/file-dirty";
import { useFileDrop } from "./state/file-drop";
import { useFileLoad } from "./state/file-load";
import { useToolbarAutohide } from "./state/toolbar-autohide";
import { AppTitle } from "./title";
Expand All @@ -21,9 +22,10 @@ export const App = () => {
useFileLoad({ editor, file });
const toolbar = useToolbarAutohide({ editor });
useEditorTheme({ editor, prefs });
const dropRef = useFileDrop({ file });

return (
<div className={s.app}>
<div className={s.app} ref={dropRef}>
Copy link
Owner

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)

<AppTitle file={file} />
<div
className={[s.toolbar, toolbar.mute ? s.muted : ""].join(" ")}
Expand Down
43 changes: 43 additions & 0 deletions src/app/state/file-drop.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { useEffect, useRef } from "react";
import { FileState } from "~src/components/file/state";

interface Params {
file: FileState;
}

export const useFileDrop = (params: Params): React.RefObject<HTMLDivElement> => {
const dropRef = useRef<HTMLDivElement>(null);

const handleDragOver = (e: DragEvent) => e.preventDefault();
Copy link
Owner

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

const handleDrop = async (e: DragEvent) => {
e.preventDefault();
if (!e.dataTransfer) return;
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");
Copy link
Owner

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

if (items && items.length) {
Copy link
Owner

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?

const file = await items[0].getAsFileSystemHandle();
if (!file) return;
try {
params.file.setHandle(file as FileSystemFileHandle);
params.file.setDirty(false);
Copy link
Owner

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

} catch (e) {
throw Error(e);
}
Copy link
Owner

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

}
};

useEffect(() => {
const { current } = dropRef;
if (!current) return;
Copy link
Owner

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

current.addEventListener("dragover", handleDragOver);
current.addEventListener("drop", handleDrop);
return () => {
current.removeEventListener("dragover", handleDragOver);
current.removeEventListener("drop", handleDrop);
};
});
Copy link
Owner

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!


return dropRef;
}