Allow to select multiple files at once from remote providers #419

sadovnychyi · 2017-11-11T10:21:20Z

Right now it's pain to select multiple files from e.g. Google Drive, since you need to constantly open the window to select another file. This PR adds a checkbox next to each file and folder.

While current uppy-server implementation isn't able to accept "folders" I believe it could be useful as well, at least for my use case – I made my own implementation of the server side on python, and it seems should be easy to download everything from a dropbox/drive folder.

I have no idea how this PR affects other providers or plugins, so please advice on that. File ID checks are a bit ugly, not sure about the best way to do that. Is it safe to just compare the provider ID? Will they ever overlap?

Should we automatically disable checkboxes if maxNumberOfFiles is set to 1?

Shift+Click to select multiple files would be cool
Additional option to disallow folder selection? At least drive has it's own mimetype for folders, so each user could just use that.
NaN as a size on folders preview
~~Maybe whole table row should act like a checkbox when you click with shift?~~

sadovnychyi · 2017-11-12T10:26:24Z

Added isFolder property for each file, so it's now possible to discard them in custom file checkers, but I'm still not sure if we should just set a mimetype for them, or keep it as another property. Google Drive has application/vnd.google-apps.folder type for folders, Dropbox has nothing, according to this text/directory shouldn't be used.

arturi · 2017-11-13T18:04:29Z

Hi! Thanks a lot for your work, it’s a useful feature indeed. Will test it out and review soon.

@ifedapoolarewaju might join in for uppy-server conversation :)

arturi · 2017-11-13T18:11:24Z

src/core/Core.js

@@ -338,6 +338,8 @@ class Uppy {
          },
          size: file.data.size || 'N/A',
          isRemote: isRemote,
+          isCheckbox: file.isCheckbox || false,
+          isFolder: file.isFolder || false,
          remote: file.remote || '',


Not sure how I feel about adding isCheckbox to a file object. It seems it’s only used to not close the provider panel when a checkbox is selected. I think we could separate the file name and checkbox is the markup and have two different click handlers for those cases.

Or maybe it’s better to not show checkboxes at all do multiselect on click, and then submit only when “done” is pressed?

isFolder is not used at the moment, because of uppy-server support, as you mentioned? Would it make sense to do this client-side, meaning that if a folder is selected, we requests it’s content and then add all files in it? Having a file that has isFolder: true feels a little weird, I see that it’s supported in Google Drive, but we have to check OneDrive, Dropbox and others as well before we go this route.

Right, isCheckbox is only used to suppress hideAllPanels function, which is always called on core:file-added event. We cannot avoid calling that event, so I'm not sure about the best solution here. Maybe we shouldn't use hideAllPanels at all?

Or maybe it’s better to not show checkboxes at all do multiselect on click, and then submit only when “done” is pressed?

This would probably solve the previous problem since we won't hide the panel automatically anymore. Hopefully you could decide on this.

I use isFolder in my custom checkRestrictions function, which isn't a perfect use case to include it in this PR, but it feels odd to exclude it.

Dropbox supports isFolder as well. The idea is that server could easily process and automatically download thousands of files, while doing this on client side would take much more time. But it seems like the later option could work with existing uppy-server API. Maybe we could come up with a default behavior which does this on client side, but with ability to pass a custom function which will process the folder on server side instead? onBeforeFileAdded could possibly be used for this.

Would it make sense to do this client-side, meaning that if a folder is selected, we requests it’s content and then add all files in it

I am in favor of this because it requires less implementation and less added complexity. Currently when a file is sent to the server for upload, a socket channel is created and sent to the client to receive upload progress. Sending a folder for the server to download would mean various socket channels would need to be created for each file in the folder so that the client can get upload progress for each file as they happen in parallel. While this is doable, I'd rather we don't add the extra complexity if there's an alternative. 🙂

The idea is that server could easily process and automatically download thousands of files, while doing this on client side would take much more time.

the actual file downloads would happen on uppy-server, we would just be fetching the metadata of all the files in the selected folder on the client side. This is already what we do when you click a folder from the dropbox/google drive view on the dashboard.

Agree, I completely forgot about sockets since I had to remove them from my server implementation (google app engine restrictions). I'll work on this. I will skip recursive folder walk to be safe, and we will need additional 'loading' state for each checkbox where it's disabled while we fetch the data.

Or maybe it’s better to not show checkboxes at all do multiselect on click, and then submit only when “done” is pressed?

BTW, I still like this idea but how do we differentiate between selection a folder and opening it?

Yes, we could differentiate on double click, but that is tricky on mobile. Let’s go with checkboxes for now.

arturi · 2017-11-13T18:12:45Z

src/plugins/Dashboard/FileItem.js

@@ -99,7 +99,7 @@ module.exports = function fileItem (props) {
        }
      </h4>
      <div class="UppyDashboardItem-status">
-        ${file.data.size && html`<div class="UppyDashboardItem-statusSize">${prettyBytes(file.data.size)}</div>`}
+        ${isNaN(file.data.size) ? '' : html`<div class="UppyDashboardItem-statusSize">${prettyBytes(file.data.size)}</div>`}


We are trying to support IE 10-11, so we’ll need a polyfill for this one, I think.

We could use lodash.isFinite for this, but again I'm not sure if they support IE10. I could just hardcore something like this but it looks messy.

ifedapoolarewaju · 2017-11-14T18:38:24Z

src/generic-provider-views/index.js

+    }
+  }
+
+  fileToId (file) {


@sadovnychyi what is the need for the fileToId method? Why not just use this.plugin.getItemId(file)?

Nice work btw 😉

this.plugin.getItemId(file) returns an ID from provider, while internally uppy uses Utils.generateFileID to construct file IDs from multiple parameters. Internal uppy ID is required to remove existing file when you uncheck the checkbox (toggleCheckbox) and to check if the current checkbox isChecked, by looking on files in uppy by their ID.

this.plugin.core.removeFile(itemId) – this one has to be internal uppy ID, not providers ID.
this.fileToId(file) in this.plugin.core.getState().files we could avoid using internal uppy ID here by comparing provider's ID for each file, but since we already use it to remove file I left it as is.

…tiselect

…elect

sadovnychyi · 2017-11-16T08:33:44Z

The checkbox state isn't updated properly anymore because of (abfd1c7#diff-24345cdf8d762d6253b9e81d7f2e8238R66). @arturi could you suggest anything since it was your change? The HTML property checked="checked" is set correctly but the UI isn't updated in time. Can I revert it back to use nanoraf or what was the reason behind that change?

sadovnychyi · 2017-11-16T10:33:04Z

Updated to fetch folder contents on checkbox change. Each checkbox is disabled while we fetch the data.

Had to bring back nanoraf -- see my previous comment. Is it fine?
file.isFolder is gone.
file.isCheckbox is still here. Please advice on a solution.
Added new local plugin state selectedFolders with {folderID: {loading: true, files: [fileId1, fileId2, ...]}, ...}
isChecked now returns an object for folders, so we can grab a "loading" state and pass it into a checkbox. We could have it in a separated method but this seems better for performance reasons. Are we cool with this?
Folder is NOT unchecked if you remove each file manually from the main screen. We could listen to core:file-removed events and update files in selectedFiles state. But should we?
~~lodash.isfinite is used in attempt to make it work with IE. Cannot test it though.~~ – this one is gone since we no longer display folders on main screen
fileToId is still here and now used even more since we often deal with "raw" files from each provider. Maybe just rename it into something like providerFileId(file)?
Should we have a notification like "Added X files from folder Y?"

arturi · 2017-11-16T22:05:45Z

The checkbox state isn't updated properly anymore because of

Yeah, tested it too, I’m able to reproduce. Haven’t got too many ideas myself, let’s keep nanoraf for now.

Reasons for removing nanoraf were:

It kept updating plugins that were .uninstalled: Calling close() on uppy instance throws error #396
With nanoraf enabled I wasn’t able to find elements via .querySelector on boot, and it was required for accessibility PR, where I need to focus on the first element when modal is open. So we decided to temporary remove it, hence abfd1c7#diff-24345cdf8d762d6253b9e81d7f2e8238R66. It’s also fixable by doing setTimeout(this.setFocusToFirstNode, 4), which is a hack, but I’ll add that for now.

arturi · 2017-11-17T04:44:08Z

Great work on folders and overall! 👌 Thank you.

file.isCheckbox is still here. Please advice on a solution.

Idea: would it make sense to keep a list of selected files in local state until “done” is pressed, and only then call addFile on that list in a loop? Then the provider tab won’t get closed.

Folder is NOT unchecked if you remove each file manually from the main screen. We could listen to core:file-removed events and update files in selectedFiles state. But should we?

And this will also be solved if we don’t store selected files permanently, but instead reset that list once “done” is pressed? I might be overseeing something though.

Should we have a notification like "Added X files from folder Y?"

I think an Informer message about this would be cool, yes.

sadovnychyi · 2017-11-17T10:48:26Z

Idea: would it make sense to keep a list of selected files in local state until “done” is pressed, and only then call addFile on that list in a loop? Then the provider tab won’t get closed.

Well this would add some extra complexity since we would have to manage two separated states.

And this will also be solved if we don’t store selected files permanently, but instead reset that list once “done” is pressed? I might be overseeing something though.

But it seems bad for UX to reset the state once done is pressed. I would rather have an option to come back to that screen to select additional files after overviewing them at main screen.

I've removed the isCheckbox core property, and we now call hideAllPanels manually after a file been added instead of listening to an event.

Fixed an error in dashboard's i18n and i18ned the "added X files from Y" message.

Also, we now listen to file removal events so we will uncheck a folder if you remove all files from the main screen. Not sure though how do we stop listening on this event in case plugin gots destroyed.

arturi · 2017-11-18T05:03:04Z

But it seems bad for UX to reset the state once done is pressed. I would rather have an option to come back to that screen to select additional files after overviewing them at main screen.

Well, depends on how you look at the UX. I do like the way it currently works, apart from the added mild code complexity.

What I meant was that we could (not saying we should) treat file selection in provider as a temporary “selection mode” thing that you do to add files “to cart”. Like you are on an e-commerce site and you clicked “add to cart” on a t-shirt, it won’t necessarily add “already in cart” badge to the t-shirt :-) Same if you click “My Device” and files from local disk, they won’t show that you’ve added those files when you return to the system file selection dialog.

Having said that, you actually went extra mile to implement this state-syncing, and I think it’s a good feature to have.

arturi · 2017-11-18T05:09:32Z

Also, we now listen to file removal events so we will uncheck a folder if you remove all files from the main screen. Not sure though how do we stop listening on this event in case plugin gots destroyed.

I guess a proper way to do it is to move that event listener to each provider’s install method, and add .off in uninstall. Any thoughts on that, @ifedapoolarewaju? It’s about a way to unset this https://github.com/sadovnychyi/uppy/blob/e6eec307839eb95066b3437a2c1948d6319dccbe/src/generic-provider-views/index.js#L74-L76 when plugin is unmounted.

I've removed the isCheckbox core property, and we now call hideAllPanels manually after a file been added instead of listening to an event.

👍

sadovnychyi · 2017-11-18T05:22:47Z

We would have to add something like ProviderPlugin with those install/uninstall methods so others (drive, dropbox, etc.) would extend from it instead of a Provider.

arturi · 2017-11-18T05:45:43Z

Well, we already have the View and Provider, wouldn’t want to introduce multiple-level inheritance, if that ProviderPlugin would have to extend regular Plugin. Maybe we can just add those listeners manually for now, and then think about refactoring it?.. 🤔

ifedapoolarewaju · 2017-11-20T08:46:38Z

I guess a proper way to do it is to move that event listener to each provider’s install method, and add .off in uninstall.

yes but this logic is more concerned with the view and not the provider. I think we could have like a tearDown(or any better name) method in the view that each provider's would call on uninstall.

So in the view we can have like

tearDown() {
  // remove listener here
}

Then in the provider plugin we have

uninstall() {
  this.view.tearDown()
  this.unmount()
}

arturi · 2017-11-20T23:14:41Z

I’m in favor of what @ifedapoolarewaju suggested:

I think we could have like a tearDown(or any better name) method in the view that each provider's would call on uninstall

Let’s do it like that, @sadovnychyi would you be able to add this to your PR? Or I can add it later after merge.

sadovnychyi · 2017-11-22T06:44:51Z

Added tearDown method.

arturi · 2017-11-22T19:57:24Z

Thank you! One last nitpick: would you be able to add a little comments in difficult places like here: https://github.com/sadovnychyi/uppy/blob/0e7505dcda86040a4663499532ecb6c0c41c8994/src/generic-provider-views/index.js#L413-L452 ? I worry that we might have hard times debugging this sometime later.

sadovnychyi · 2017-11-23T08:18:35Z

Added some docstrings. I think understanding the basic idea of a method should be enough.

arturi · 2017-11-23T22:53:41Z

Thank you! Merged.

sadovnychyi added 5 commits November 11, 2017 18:12

Allow to select multiple files at once from remote providers

ef0f708

Fix for styleguide

e976d1f

Allow to select multiple files using shift and click

cca123b

Do not display NaN for folder sizes

5e8d5cf

Add isFolder property for each file

0f12dc8

arturi self-requested a review November 13, 2017 18:04

arturi reviewed Nov 13, 2017

View reviewed changes

ifedapoolarewaju reviewed Nov 14, 2017

View reviewed changes

sadovnychyi and others added 4 commits November 16, 2017 10:37

Merge branch 'master' into multiselect

37bbaa2

Merge branch 'master' of https://github.com/transloadit/uppy into mul…

258343c

…tiselect

Merge branch 'multiselect' of github.com:sadovnychyi/uppy into multis…

70471d4

…elect

Use lodash.isFinite instead of isNaN for IE support

2718837

sadovnychyi added 2 commits November 16, 2017 18:21

Add/remove folder contents on checkbox change

e8971eb

Remove isFolder property from files

5f17d0d

No need to check for isFinite since folders are no longer displayed

f1dd42c

sadovnychyi added 8 commits November 17, 2017 15:24

Handle errors when adding a folder

f046d3d

Add info message with number of files added from folder

35b1624

fileToId -> providerFileToId

47b0434

Remove isCheckbox core property; Manually call hideAllPanels instead

998178d

Remove unused argument

0bde308

Update folder state when file is removed from main screen

5c94437

Fix wrong i18n assignment in dashboard

4884428

Add i18n for folder message

912b6b8

Add another message for empty folders

e6eec30

sadovnychyi and others added 3 commits November 22, 2017 13:59

Merge branch 'master' into multiselect

1f1f064

Fix nanoraf

46b53f5

Add tearDown method for provider view

0e7505d

Add some docs for new methods

ffbcfdc

arturi merged commit c319d32 into transloadit:master Nov 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to select multiple files at once from remote providers #419

Allow to select multiple files at once from remote providers #419

sadovnychyi commented Nov 11, 2017 •

edited

Loading

sadovnychyi commented Nov 12, 2017

arturi commented Nov 13, 2017

arturi Nov 13, 2017

arturi Nov 13, 2017

sadovnychyi Nov 14, 2017 •

edited

Loading

ifedapoolarewaju Nov 14, 2017

sadovnychyi Nov 16, 2017

sadovnychyi Nov 16, 2017

arturi Nov 18, 2017

arturi Nov 13, 2017

sadovnychyi Nov 14, 2017

ifedapoolarewaju Nov 14, 2017

sadovnychyi Nov 16, 2017

sadovnychyi commented Nov 16, 2017

sadovnychyi commented Nov 16, 2017 •

edited

Loading

arturi commented Nov 16, 2017

arturi commented Nov 17, 2017

sadovnychyi commented Nov 17, 2017

arturi commented Nov 18, 2017

arturi commented Nov 18, 2017

sadovnychyi commented Nov 18, 2017

arturi commented Nov 18, 2017

ifedapoolarewaju commented Nov 20, 2017 •

edited

Loading

arturi commented Nov 20, 2017

sadovnychyi commented Nov 22, 2017

arturi commented Nov 22, 2017 •

edited

Loading

sadovnychyi commented Nov 23, 2017

arturi commented Nov 23, 2017

Allow to select multiple files at once from remote providers #419

Allow to select multiple files at once from remote providers #419

Conversation

sadovnychyi commented Nov 11, 2017 • edited Loading

sadovnychyi commented Nov 12, 2017

arturi commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadovnychyi Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadovnychyi commented Nov 16, 2017

sadovnychyi commented Nov 16, 2017 • edited Loading

arturi commented Nov 16, 2017

arturi commented Nov 17, 2017

sadovnychyi commented Nov 17, 2017

arturi commented Nov 18, 2017

arturi commented Nov 18, 2017

sadovnychyi commented Nov 18, 2017

arturi commented Nov 18, 2017

ifedapoolarewaju commented Nov 20, 2017 • edited Loading

arturi commented Nov 20, 2017

sadovnychyi commented Nov 22, 2017

arturi commented Nov 22, 2017 • edited Loading

sadovnychyi commented Nov 23, 2017

arturi commented Nov 23, 2017

sadovnychyi commented Nov 11, 2017 •

edited

Loading

sadovnychyi Nov 14, 2017 •

edited

Loading

sadovnychyi commented Nov 16, 2017 •

edited

Loading

ifedapoolarewaju commented Nov 20, 2017 •

edited

Loading

arturi commented Nov 22, 2017 •

edited

Loading