-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Files] Download multiple files in a zip #1459
Conversation
…om/ChainSafe/files-ui into feat/unblock-ui-while-sharing-1377
…haredFolderModal.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…eCreateOrEditSharedFolder.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…/index.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…/TransferToast.tsx Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…om/ChainSafe/files-ui into feat/unblock-ui-while-sharing-1377
Hey Thibaut, this is working really nicely. A couple of small observations, some may not be issues but I wanted to mention them and let you decide. a) It looks like the "download complete" modal is using the wrong colour icon (but I'm guessing this is the same one we always use and probably was pre-existing before these change). b) I noticed that the option doesn't appear to download a folder as a zip (even though it contains files) if that's the only item selected on the root c) If you attempt to start a file upload at the same time the two toasts will overlap each other and do not stack Screen.Recording.2021-08-20.at.9.39.57.AM.movd) The option to download as a zip is missing from the kebab menu (makes sense to have it for folders containing items?). e) It's not possible to use this feature on a mobile device (because the checkboxes don't appear in the mobile view) f) It's not possible to use this feature when the file browser is in grid mode (also because there are checkboxes in that view) g) Once the selection has been made and the download has began would it make sense to disable the button until a new selection has been made? I'm guessing it would be pretty silly for someone to spam click the download button but it is possible. I was just not sure if this could be used maliciously to generate a lot of load and cause us issues? For example a 10gb collection of files spammed clicked hundreds of times. |
aah lot's of good points. Thanks for stress testing this. This feature is actually reusing the download toast, some of what you mention would also happen without this feature and require a better design, namely
Preventing the click when there's a download makes sense, although I'm not sure, because we specifically don't want to block the UI, if someone wants to be malicious, they can fire programmatically 1000 calls still. edit: what I'll do is remove the selection then, I guess that should be fine. What I'll do
That's no me, but small enough indeed
Didn't think about it either, I will allow it.
Same, didn't think about it, totally makes sense. |
Great, sounds good 👍. I've created separate issues for the other things discovered in testing but not introduced by this PR #1465 and #1466 De-selecting the items after initiating the download sounds like a good solution too. |
…t-download-all-1363
…i-monorepo into feat/tbaut-download-all-1363
Added the following:
|
Added the following:
|
The new additional changes are looking almost perfect.
👆 just a small issue with this - the menu option is there and works as intended if the folder contains files. However, if the folder is empty the option is still there but nothing happens then the option is clicked. Perhaps it'd be best to not show the option if the folder is empty or send feedback to the user if they are attempting to download a blank folder, what do you think? Everything else is looking great |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that was a beautiful experience 🔥
Good point, having nothing happening is def wrong. The problem with what you propose is that the users will first think it's a bug, because "why isn't the menu entry present for this folder, but it is for this one"? So I'll add an error toast, I think it's more explicit (it's also makes the whole logic a bit more convoluted but that's a needed evil I guess). |
👍 Nice solution, works well |
closes #1363