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

Support Drag and Drop of Folders as well as files, fixes #668 #659

Merged
merged 7 commits into from May 8, 2017

Conversation

Projects
None yet
4 participants
@humphd
Member

humphd commented Mar 22, 2017

I saw this tweet yesterday about CodePen's ability to let you drag and drop folders as well as files:

https://twitter.com/CodePen/status/843908650997600256?s=03

When I wrote our drag and drop stuff, browsers only supported files. So I did some research, and sure enough, Edge, Chrome, and Firefox all support it (hilariously, all via a webkit* prefix method). The drag and drop and file api specs are a bit of a dog's breakfast, however, I was able to make it all work.

With this patch, I've reworked our code to support both the new webkit way (which supports folders and files) and also the old way, which only works with folders. Both ways still support auto-expanding of .zip and .tar files contained within.

I could use some help really testing this on various browsers and files/folders. cc @Pomax and @flukeout.

@Blackweda

This comment has been minimized.

Show comment
Hide comment
@Blackweda

Blackweda Mar 23, 2017

Your build failed around the eslint:src section.. my PR also failed around that sort of thing. How do we fix that??

Blackweda commented Mar 23, 2017

Your build failed around the eslint:src section.. my PR also failed around that sort of thing. How do we fix that??

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 23, 2017

Member

@Blackweda you just have to fix the lint errors. My code has some, it seems. I'll fix and push a patch. You can test it locally first to make sure lint passes.

Member

humphd commented Mar 23, 2017

@Blackweda you just have to fix the lint errors. My code has some, it seems. I'll fix and push a patch. You can test it locally first to make sure lint passes.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 25, 2017

Member

I also added another commit that fixes mozilla/thimble.mozilla.org#1886, allowing you to drag files/folders onto an existing dir vs. always importing to the project root. My code also auto-opens subdirs as you hover over them, in case they are closed.

Member

humphd commented Mar 25, 2017

I also added another commit that fixes mozilla/thimble.mozilla.org#1886, allowing you to drag files/folders onto an existing dir vs. always importing to the project root. My code also auto-opens subdirs as you hover over them, in case they are closed.

@humphd humphd requested a review from Pomax Mar 25, 2017

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 25, 2017

Member

@Pomax this PR is mostly dedicated to you. I should get you to test it and see if it works well for your use cases. Some things to try/note:

  • Only some browsers support dragging folders in addition to files. This patch supports whatever the browser allows, using 1 of 2 import strategies.
  • Dragging and dropping .zip and .tar files still works as before (auto-expanding them)
  • I've made it possible to drag-and-drop deep into the filetree. If you hover over a dir, it will open it, allowing you to navigate the tree, then drop, and it will use the path you drop on as the root for importing vs. the project root.

I don't have access to Windows and IE/Edge, so I don't know what it does in these browsers. But on Mac, it's working in Safari, Chrome, Firefox, and Opera.

I'm fairly confident that I can do 2 more patches to follow this that will support:

  • dragging files out of the browser onto your desktop (only Chrome supports this at the moment, but it's very cool, and what Gmail uses)
  • dragging files around in the file tree to move them.

I've now gotten the hang of the filetree code :)

Member

humphd commented Mar 25, 2017

@Pomax this PR is mostly dedicated to you. I should get you to test it and see if it works well for your use cases. Some things to try/note:

  • Only some browsers support dragging folders in addition to files. This patch supports whatever the browser allows, using 1 of 2 import strategies.
  • Dragging and dropping .zip and .tar files still works as before (auto-expanding them)
  • I've made it possible to drag-and-drop deep into the filetree. If you hover over a dir, it will open it, allowing you to navigate the tree, then drop, and it will use the path you drop on as the root for importing vs. the project root.

I don't have access to Windows and IE/Edge, so I don't know what it does in these browsers. But on Mac, it's working in Safari, Chrome, Firefox, and Opera.

I'm fairly confident that I can do 2 more patches to follow this that will support:

  • dragging files out of the browser onto your desktop (only Chrome supports this at the moment, but it's very cool, and what Gmail uses)
  • dragging files around in the file tree to move them.

I've now gotten the hang of the filetree code :)

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Mar 27, 2017

I tried this with a folder on my desktop called "test" containing C5th86IUoAA-T9w.jpg, a 400KBish JPG, gauss.txt, a 1.5KB plain text file, and test.txt, an 8 byte plain text file. When I drag this into the file tree in both Chrome 56 and Firefox 52 (using Windows 10 Pro x64) the directory gets built, but only the JPG file shows as dir content. The .txt files do not show.

Pomax commented Mar 27, 2017

I tried this with a folder on my desktop called "test" containing C5th86IUoAA-T9w.jpg, a 400KBish JPG, gauss.txt, a 1.5KB plain text file, and test.txt, an 8 byte plain text file. When I drag this into the file tree in both Chrome 56 and Firefox 52 (using Windows 10 Pro x64) the directory gets built, but only the JPG file shows as dir content. The .txt files do not show.

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Mar 27, 2017

Edge 14 does not like folder drag-and-drop, it would appear. I can't drop a folder onto the filetree in it.

Pomax commented Mar 27, 2017

Edge 14 does not like folder drag-and-drop, it would appear. I can't drop a folder onto the filetree in it.

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Mar 27, 2017

do we filter on extension @humphd? if I rename my .txt files to .js, then they do end up in the filetree...

Pomax commented Mar 27, 2017

do we filter on extension @humphd? if I rename my .txt files to .js, then they do end up in the filetree...

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 27, 2017

Member

That *.txt vs. *.js issue is new to me, and certainly looks like a bug. I've filed it separate to this: #668.

Folder drag and drop is not going to work in IE/Edge (yet), no, so that's known. Dragging a bunch of files should work, though, in all browsers.

Member

humphd commented Mar 27, 2017

That *.txt vs. *.js issue is new to me, and certainly looks like a bug. I've filed it separate to this: #668.

Folder drag and drop is not going to work in IE/Edge (yet), no, so that's known. Dragging a bunch of files should work, though, in all browsers.

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Mar 27, 2017

barring that weird extension filtering, this actually seems to work pretty well. I tried dragging the folder into itself a few times. Works pretty well too - does highlight a question though: do we want a file-overwrite warning when a drop operation will overwrite existing files?

Pomax commented Mar 27, 2017

barring that weird extension filtering, this actually seems to work pretty well. I tried dragging the folder into itself a few times. Works pretty well too - does highlight a question though: do we want a file-overwrite warning when a drop operation will overwrite existing files?

@Pomax

This comment has been minimized.

Show comment
Hide comment
@Pomax

Pomax Mar 28, 2017

if the overwrite issue is known and we have issues outstanding, then this is R+ from me.

Pomax commented Mar 28, 2017

if the overwrite issue is known and we have issues outstanding, then this is R+ from me.

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd Mar 29, 2017

Member

Thanks for the reviews. I'm going to wait on a few other PRs to land that touch this code, and then I'll circle back, rebase, and try to get this in.

Member

humphd commented Mar 29, 2017

Thanks for the reviews. I'm going to wait on a few other PRs to land that touch this code, and then I'll circle back, rebase, and try to get this in.

@humphd humphd requested a review from gideonthomas May 8, 2017

@humphd

This comment has been minimized.

Show comment
Hide comment
@humphd

humphd May 8, 2017

Member

@gideonthomas I've rebased this, and fixed a few things I noticed in testing. Do you want to take a look before I land?

Member

humphd commented May 8, 2017

@gideonthomas I've rebased this, and fixed a few things I noticed in testing. Do you want to take a look before I land?

@gideonthomas

This comment has been minimized.

Show comment
Hide comment
@gideonthomas
Member

gideonthomas commented May 8, 2017

sure

@gideonthomas

Mainly just nits in the code!

I tested this out and it works well! One thing I noticed was that in Thimble, while dnd'ing folders/files/archives works perfectly if you do it directly even if there is a file in the folder that exceeds that max file size limit. However, if we use the "Upload Files" dialog and drag a folder (or archive or just the file) containing a file that is more than the max file size, it shows the error and then the editor just hangs. I feel like that is a known issue, but I'm not sure. If not, let's file a follow-up to fix in Thimble since we need to fix the strings in the "Upload Files" dialog as well (since we can upload folders now too)

Show outdated Hide outdated src/filesystem/impls/filer/lib/LegacyFileImport.js
Show outdated Hide outdated src/filesystem/impls/filer/lib/WebKitFileImport.js
@@ -538,7 +551,8 @@ define(function (require, exports, module) {
className: this.getClasses("jstree-leaf"),
onClick: this.handleClick,
onMouseDown: this.handleMouseDown,
onDoubleClick: this.handleDoubleClick
onDoubleClick: this.handleDoubleClick,
onDragEnter: this.handleDragEnter

This comment has been minimized.

@gideonthomas

gideonthomas May 8, 2017

Member

it's interesting that we don't need to bind to this here even though we use this inside handleDragEnter. Maybe it does .call(this, ...)?

@gideonthomas

gideonthomas May 8, 2017

Member

it's interesting that we don't need to bind to this here even though we use this inside handleDragEnter. Maybe it does .call(this, ...)?

Show outdated Hide outdated src/utils/DragAndDrop.js

@humphd humphd changed the title from Support Drag and Drop of Folders as well as files to Support Drag and Drop of Folders as well as files, fixes #668 May 8, 2017

@humphd humphd merged commit 906bf7b into mozilla:master May 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment