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

feat(upload): handle uploading folders and multiple files #44

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Dorosty
Copy link

@Dorosty Dorosty commented Jan 23, 2024

  • requires feat(dnd): add configurable dataTransferProperty to droppable osjs-client#213
  • progressDialog has been changed to take a string instead of a file, so that when we are uploading multiple files, the title of the dialog would be 'Uploading multiple files...'
  • uploadBrowserFiles has been completely re-implemented to allow uploading multiple files, and also recursively handling folders and their content.

@Dorosty Dorosty changed the title fix(dnd): handle uploading folders and multiple files fix(upload): handle uploading folders and multiple files Jan 23, 2024
@Dorosty Dorosty changed the title fix(upload): handle uploading folders and multiple files ffeat(upload): handle uploading folders and multiple files Jan 23, 2024
@Dorosty Dorosty changed the title ffeat(upload): handle uploading folders and multiple files feat(upload): handle uploading folders and multiple files Jan 23, 2024
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Dorosty
Copy link
Author

Dorosty commented Feb 27, 2024

This branch #45 adds support for uploading folders from the menu. Hopefully it can be merged after this.

@Dorosty
Copy link
Author

Dorosty commented Mar 11, 2024

If there's anything I can do to help make this easier to review, please let me know (e.g. breaking this PR into smaller ones)

@andersevenrud
Copy link
Member

This looks pretty good @Dorosty! Haven't had a chance to fully very everything because I started a new job last week and been insanely busy. Things are now finally cooling down though, so I'll take a closer look and get back to you ASAP.

@andersevenrud
Copy link
Member

Another contributor brought up a relevant issue recently: os-js/OS.js#847

@Dorosty
Copy link
Author

Dorosty commented Mar 18, 2024

This looks pretty good @Dorosty! Haven't had a chance to fully very everything because I started a new job last week and been insanely busy. Things are now finally cooling down though, so I'll take a closer look and get back to you ASAP.

Congratulations & TYSM 🙏

@Dorosty
Copy link
Author

Dorosty commented Mar 18, 2024

Another contributor brought up a relevant issue recently: os-js/OS.js#847

This would require a change in the system vfs adapter in osjs-client, which would require multiple PRs. May I suggest we merge this one first & then I will add the cancellability feature in separate PRs.

@andersevenrud
Copy link
Member

May I suggest we merge this one first

Hmm. I kinda feel like cancellation should be added first, but there's nothing really in the way to take this one first.

@Dorosty
Copy link
Author

Dorosty commented Apr 1, 2024

May I suggest we merge this one first

Hmm. I kinda feel like cancellation should be added first, but there's nothing really in the way to take this one first.

@Dorosty
Copy link
Author

Dorosty commented Apr 20, 2024

May I suggest we merge this one first

Hmm. I kinda feel like cancellation should be added first, but there's nothing really in the way to take this one first.

This is done BTW

@andersevenrud
Copy link
Member

andersevenrud commented Apr 27, 2024

Finally found some time to test this out (on various platforms). Seems to be working great!

I just have one final comment that came to mind while testing. It would be really nice if this feature could be set via a configuration that defaults to true (like filemanager.disableDownload). Could really make use of that on my demo server :)

@Dorosty
Copy link
Author

Dorosty commented Apr 28, 2024

I just have one final comment that came to mind while testing. It would be really nice if this feature could be set via a configuration that defaults to true (like filemanager.disableDownload). Could really make use of that on my demo server :)

Would this be a good config schema?

filemanager: {
  uploadMethod: 'single' | 'multiple',
}

@andersevenrud
Copy link
Member

Just a simple boolean would suffice: filemanager.disableMultiUpload or something like that.

@Dorosty
Copy link
Author

Dorosty commented May 5, 2024

Just a simple boolean would suffice: filemanager.disableMultiUpload or something like that.

Done! 🎉 please note that os-js/osjs-client#216 should also be merged for AbortController to work.

@@ -48,6 +48,14 @@ import {
listView
} from '@osjs/gui';


/**
* flag indicating whether uploading folders is supported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you capitalize this comment? Other than that, looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants