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: add unzip extension #39

Merged
merged 1 commit into from
Aug 5, 2024
Merged

feat: add unzip extension #39

merged 1 commit into from
Aug 5, 2024

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Jul 30, 2024

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

😍

@JammingBen JammingBen self-assigned this Jul 30, 2024
@JammingBen JammingBen force-pushed the feat/unzip-extension branch 2 times, most recently from 51f1563 to 042f6b1 Compare July 30, 2024 14:08
packages/web-app-unzip/types.d.ts Outdated Show resolved Hide resolved
@JammingBen JammingBen force-pushed the feat/unzip-extension branch 6 times, most recently from eaebc34 to 7680828 Compare July 31, 2024 13:43
@JammingBen JammingBen marked this pull request as ready for review August 1, 2024 05:22
@JammingBen JammingBen force-pushed the feat/unzip-extension branch 2 times, most recently from 474fcb4 to 7efbcb6 Compare August 2, 2024 05:20
@AlexAndBear
Copy link
Contributor

AlexAndBear commented Aug 2, 2024

Some nitpicks:

  • A sample file of 50mb takes super long, and gives the impression that the feature is broken (which isn't the case), we could think of decreasing the threshold drastically to 10MB or maybe show a message that it might take longer
  • i18n is missing
  • unit tests are missing
  • doesn't work with the local docker setup
  • getting a csp error when running via Ocis 9200 instead of 9201 in web (docker)

Other than that: 🚀

@JammingBen
Copy link
Contributor Author

  • A sample file of 50mb takes super long, and gives the impression that the feature is broken (which isn't the case), we could think of decreasing the threshold drastically to 10MB or maybe show a message that it might take longer

@tbsbdr What do you think? I kinda like the idea of showing a message that it might take a while.

  • i18n is missing

Yep, we should definitely get this going for this repo: #40

  • unit tests are missing

👍

  • doesn't work with the local docker setup

I just tried it, the app gets correctly shipped by oCIS for me. Extracting doesn't work though because the Web version bundled with oCIS is not up to date. Also, I get an error in the cast app: Cannot destructure property 'resources' of 'undefined' as it is undefined..

  • getting a csp error when running via Ocis 9200 instead of 9201 in web (docker)

True, that should be fixed.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Works perfectly - I have one UX related question though:

  1. zip a subfolder of the linux folder (alchemy in mips)
  2. upload alchemy.zip to personal space
  3. right click and Extract here

expected behaviour

Folder alchemy gets created as root level wrapping folder where the content of the zip file gets extract to.

actual behaviour

Folder alchemy (1) gets created as root level wrapping folder, although there is no other content than the alchemy.zip file.

packages/web-app-unzip/src/composables/useExtensions.ts Outdated Show resolved Hide resolved
@JammingBen JammingBen merged commit 619bde4 into main Aug 5, 2024
1 check passed
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.

[web] Unzip in Web
4 participants