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

Improve drag to upload state #1440

Merged
merged 9 commits into from Apr 18, 2019

Conversation

Projects
None yet
3 participants
@lakesare
Copy link
Collaborator

commented Apr 10, 2019

This uncovered a few drag-drop issues we have, it comes down to us needing something other than the drag-drop library.

Previous situation (in master now):

  • grey bg on dragover
  • flickering of a class in all browsers (usually not noticeable because it was just grey bg)
  • no dropover designs at all on Safari (screen only becomes grey on dragover when UrlPlugin is used)

Current situation:

  • beautiful bg on dragover
  • no flickering of a class
  • no dropover designs at all on Safari (ever)
@lakesare

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 15, 2019

Good parts

Currently checked and working:

Chrome

  • paste from browser (single)
  • paste from device (single, multiple won't work, only the last item will get pasted)
  • drop from browser (single)
  • drop from device (single)
  • drop from folder in device

Firefox

  • paste from browser (single)
  • paste from device (single)
  • drop from browser (single)
  • drop from device (single)
  • drop from folder in device

Safari

  • paste from browser (single)
  • paste from device (single, multiple works)
  • drop from browser (single)
  • drop from device (single)
  • drop from folder in device

On dragOver there is a nonflickering placeholder alex created, everywhere.
Dragover class never flickers either.

Concerns

getDroppedFiles's apis (webkitGetAsEntryApi, getFilesAndDirectoriesApi, fallbackApi) were partially rewritten from the source, but they are still not easy to read.
We could rewrite them further, and maybe make them return a promise that would be resolving to files instead of utilizing a callback.
webkitGetAsEntryApi is what gets called in the latest versions of Chrome, Firefox and Safari. I don't have a browser to check getFilesAndDirectoriesApi. fallbackApi works well in all of these browsers too.

@lakesare

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 15, 2019

@arturi, could you please take a look at it?
What do you think about refactoring webkitGetAsEntryApi and getFilesAndDirectoriesApi, is it worth it right now?

@arturi

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Awesome work, thank you!

@arturi

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Tried pasting a url — nothing happens.

@arturi

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

We once had a PR that introduced folders-dropped event: https://github.com/transloadit/uppy/pull/849/files. Can we leave a room for this here, either add this event, or somehow preserve original path of the folder that was dropped? This feature is needed if one wants to re-create user’s file system on the server side.

The other interesting part could be then to pass the files to the backend with all the infos that makes possible, if desired, to recreate the directory tree of the user. Chrome and Firefox exposes the webkitRelativePath parameter in the File object to accomplish this.

Maybe we could just add webkitRelativePath as file meta 🤔

@lakesare

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2019

@arturi, I like the idea of adding it as a file meta! I'll add it.

Do you think this catch is needed:

image

If we don't have it, there is just an error logged to the console, which's not a big deal (and probably helpful for debugging). Do you think we'll have some bad consequences from not catching errors for this.uppy.addFile()?
Or maybe could we be catching all errors in this.uppy.addFile method instead of repeating try catch across the codebase?

@arturi

This comment has been minimized.

Copy link
Collaborator

commented Apr 17, 2019

Do you think this catch is needed:

I’d ask @goto-bus-stop to answer this, I think we had some problems without the catch, do you remember?

@lakesare

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

Current Situation

Chrome, Firefox

  • from browser [Copy Image Address]
  • from browser [Copy Image]
  • from browser [drag image]
  • from device [drag file]
  • from device [drag folder]
  • from device [copy file]

Safari

  • from browser [Copy Image Address]
  • from browser [Copy Image]
  • from browser [drag image] --- works from safari to safari, fails from chrome to safari (dropping from safari to chrome works), but I think that's fine
  • from device [drag file]
  • from device [drag folder]
  • from device [copy file]
@arturi

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Thanks again for the awesome work, Evgenia! Merging this so it can make it to the next release. Please remind me and Renee about try/catch thing, and as for getDroppedFiles APIs and rewriting them further, I think it’s good enough for now. Thanks!

@arturi arturi merged commit cb27df2 into transloadit:master Apr 18, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.