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

PartialTree - change the maxTotalFileSize error #5203

Merged
merged 3 commits into from
May 30, 2024

Conversation

lakesare
Copy link
Contributor

@lakesare lakesare commented May 29, 2024

This is a series of PRs that make the #5050 PR smaller


Currently, exceeding the aggregate size of files is reported as "hi.png exceeds maximum allowed size of 20 MB", where "hi.png" is the first file that overflows the max allowed size (there might be many files after this first file!).

This PR changes this phrasing to "You selected 25 MB of files, but maximum allowed size is 20 MB".

The former phrasing looks a bit better when we're hovering over greyed-out files, however in #5050 UI we won't have greyed-out files, so we shouldn't worry about that.

The new phrasing makes more sense as an error notification, both in current UI and in #5050 UI.

Note

I understand it seems like we should change "You selected %{size} of files, but maximum allowed size is %{sizeAllowed}" to "You want to select %{size} of files, but maximum allowed size is %{sizeAllowed}", however the former phrasing fits the #5050 UI better.

Before

image image

After

image image

Copy link
Contributor

github-actions bot commented May 29, 2024

Diff output files
diff --git a/packages/@uppy/core/lib/Restricter.js b/packages/@uppy/core/lib/Restricter.js
index 7a5ac8b..0dd13c1 100644
--- a/packages/@uppy/core/lib/Restricter.js
+++ b/packages/@uppy/core/lib/Restricter.js
@@ -53,22 +53,17 @@ class Restricter {
       }
     }
     if (maxTotalFileSize) {
-      let totalFilesSize = existingFiles.reduce((total, f) => {
+      const totalFilesSize = [...existingFiles, ...addingFiles].reduce((total, f) => {
         var _f$size;
         return total + ((_f$size = f.size) != null ? _f$size : 0);
       }, 0);
-      for (const addingFile of addingFiles) {
-        if (addingFile.size != null) {
-          totalFilesSize += addingFile.size;
-          if (totalFilesSize > maxTotalFileSize) {
-            throw new RestrictionError(
-              this.getI18n()("exceedsSize", {
-                size: prettierBytes(maxTotalFileSize),
-                file: addingFile.name,
-              }),
-            );
-          }
-        }
+      if (totalFilesSize > maxTotalFileSize) {
+        throw new RestrictionError(
+          this.getI18n()("aggregateExceedsSize", {
+            sizeAllowed: prettierBytes(maxTotalFileSize),
+            size: prettierBytes(totalFilesSize),
+          }),
+        );
       }
     }
   }
diff --git a/packages/@uppy/core/lib/locale.js b/packages/@uppy/core/lib/locale.js
index d0707fb..cdb4f7c 100644
--- a/packages/@uppy/core/lib/locale.js
+++ b/packages/@uppy/core/lib/locale.js
@@ -12,6 +12,7 @@ export default {
       0: "You have to select at least %{smart_count} file",
       1: "You have to select at least %{smart_count} files",
     },
+    aggregateExceedsSize: "You selected %{size} of files, but maximum allowed size is %{sizeAllowed}",
     exceedsSize: "%{file} exceeds maximum allowed size of %{size}",
     missingRequiredMetaField: "Missing required meta fields",
     missingRequiredMetaFieldOnFile: "Missing required meta fields in %{fileName}",

@lakesare lakesare marked this pull request as ready for review May 29, 2024 03:42
@lakesare lakesare requested review from mifi and Murderlon May 29, 2024 03:43
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

LGTM, one request

packages/@uppy/core/src/locale.ts Show resolved Hide resolved
@lakesare lakesare merged commit 31cc47f into main May 30, 2024
17 checks passed
@lakesare lakesare deleted the lakesare/partialTree-aggregateExceedsSize branch May 30, 2024 02:17
@github-actions github-actions bot mentioned this pull request Jun 4, 2024
github-actions bot added a commit that referenced this pull request Jun 4, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3-multipart |  3.12.0 | @uppy/webcam           |   3.4.2 |
| @uppy/core             |  3.12.0 | uppy                   |  3.26.0 |
| @uppy/transloadit      |   3.7.0 |                        |         |

- meta: remove Companion's `prepublishOnly` (Mikael Finstad / #5220)
- docs: document clearUploadedFiles (Merlijn Vos / #5204)
- @uppy/webcam: add missing types for `recordedVideo` (Antoine du Hamel / #5208)
- @uppy/core: check capabilities in clearUploadedFiles (Merlijn Vos / #5201)
- @uppy/core: PartialTree - change the `maxTotalFileSize` error (Evgenia Karunus / #5203)
- @uppy/transloadit: remove `updateNumberOfFilesInAssembly` (Merlijn Vos / #5202)
- @uppy/aws-s3: resolve all headers on response (Merlijn Vos / #5195)
- docs: Improve provider docs: OneDrive (Evgenia Karunus / #5196)
Murderlon added a commit that referenced this pull request Jun 5, 2024
* 4.x:
  meta: fix AWS test suite (#5229)
  Release: uppy@4.0.0-beta.10 (#5225)
  Release: uppy@3.26.0 (#5223)
  meta: remove Companion's `prepublishOnly` (#5220)
  docs: document clearUploadedFiles (#5204)
  @uppy/webcam: add missing types for `recordedVideo` (#5208)
  @uppy/core: check capabilities in clearUploadedFiles (#5201)
  PartialTree - change the `maxTotalFileSize` error (#5203)
  @uppy/transloadit: remove `updateNumberOfFilesInAssembly` (#5202)
  @uppy/aws-s3: resolve all headers on response (#5195)
  Improve provider docs: OneDrive (#5196)
Murderlon added a commit that referenced this pull request Jun 10, 2024
* main: (262 commits)
  Release: uppy@3.26.0 (#5223)
  meta: remove Companion's `prepublishOnly` (#5220)
  docs: document clearUploadedFiles (#5204)
  @uppy/webcam: add missing types for `recordedVideo` (#5208)
  @uppy/core: check capabilities in clearUploadedFiles (#5201)
  PartialTree - change the `maxTotalFileSize` error (#5203)
  @uppy/transloadit: remove `updateNumberOfFilesInAssembly` (#5202)
  @uppy/aws-s3: resolve all headers on response (#5195)
  Improve provider docs: OneDrive (#5196)
  Release: uppy@3.25.5 (#5193)
  @uppy/transloadit: do not cancel assembly when removing all files (#5191)
  @uppy/xhr-upload: fix regression for lowercase HTTP methods (#5179)
  meta: improve changelog generator (#5190)
  Release: uppy@3.25.4 (#5188)
  @uppy/svelte: do not attempt removing plugin before it's created (#5186)
  Update facebook.mdx
  fixup! @uppy/tus: fix no headers passed to companion if argument is a function (#5182)
  @uppy/tus: fix no headers passed to companion if argument is a function (#5182)
  @uppy/companion: fix google drive gsuite export large size (#5144)
  Improve provider docs: Box & Zoom (#5166)
  ...
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