Disable drop state for non-files #2449
Conversation
|
Hi! Thank you for the PR! Could you add the same check to https://github.com/transloadit/uppy/blob/master/packages/%40uppy/drag-drop/src/index.js#L118, please? |
|
Thanks for the PR—I just added a few compatibility tweaks. There is one more issue. We currently support dropping URLs straight onto the Dashboard if you're also using the That feature is implemented by having the Dashboard cal the Maybe what we need is another such method, |
| // 1. Check if the "type" of the datatransfer object includes files | ||
| const { types } = event.dataTransfer | ||
| const hasFiles = types.some(type => type === 'Files') | ||
|
|
||
| // 2. Check if some plugin can handle the datatransfer without files — | ||
| // for instance, the Url plugin can import a url | ||
| let somePluginCanHandleRootDrop = true | ||
| this.uppy.iteratePlugins((plugin) => { | ||
| if (plugin.canHandleRootDrop?.(event)) { | ||
| somePluginCanHandleRootDrop = true | ||
| } | ||
| }) | ||
|
|
||
| // 3. Deny drop, if no plugins can handle datatransfer, there are no files, | ||
| // or when opts.disabled is set, or new uploads are not allowed | ||
| if ( | ||
| (!somePluginCanHandleRootDrop && !hasFiles) | ||
| || this.opts.disabled | ||
| // opts.disableLocalFiles should only be taken into account if no plugins | ||
| // can handle the datatransfer | ||
| || (this.opts.disableLocalFiles && (hasFiles || !somePluginCanHandleRootDrop)) | ||
| || !this.uppy.getState().allowNewUpload | ||
| ) { | ||
| event.dataTransfer.dropEffect = 'none' | ||
| clearTimeout(this.removeDragOverClassTimeout) |
There was a problem hiding this comment.
Can we put this into a function? Doesn't necessarily need to be outside of this function, can be in the same lexical scope.
A pattern I'm starting to dislike in Uppy is class methods growing bigger and bigger, doing multiple things, and it's really hard to follow line by line what's happening. By grouping every computation or check, it makes things much easier to skim if it has an intention revealing name.
For example:
if (!checkUserOptions()) {
return
}
if (!checkEventForFiles() && !checkPluginSupportForEvent()) {
return
}Looking at this, I don't even need to go into those functions if that's not relevant for what I might be doing there, it already tells me what is happening without thinking too hard.
|
|
||
| // 4. Add all dropped files, handle errors | ||
| let executedDropErrorOnce = false |
There was a problem hiding this comment.
won't this be false every time handleDrop is called?
| const files = await getDroppedFiles(event.dataTransfer, { logDropError }) | ||
| this.addFiles(files) | ||
| getDroppedFiles(event.dataTransfer, { logDropError }) | ||
| .then((files) => { |
| if (this.opts.onDrop) this.opts.onDrop(event) | ||
|
|
||
| handleDragOver (event) { | ||
| this.opts?.onDragOver(event) |
There was a problem hiding this comment.
should be moved to the bottom.
* main: Disable drop state for non-files (#2449) meta: use Yarn v3 instead of npm (#3237) examples: Update to Parcel 2 (#3275) ci: test Companion with Node.js 16.x (#3273) e2e tests: Move transloadit2 test into transloadit, remove the former (#3271) Release ci: fix tranloadit2 e2e tests (#3270) Add unique class to discard button (#3269)
Currently, the dashboard shows the "Drop files here" UI even if the content that is dragged over is not a file. This also happens if the user tries to drag & drop items directly from Outlook onto the drop area using Firefox (see this bug report for details).
This PR checks if the types collection contains the value
Files. In this case, at least one of the items will be a file that can be further processed by Uppy.