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

Add a @minFileSizeKB argument to the <AuFileUpload> component #532

Merged
merged 7 commits into from
Feb 13, 2025

Conversation

aatauil
Copy link
Contributor

@aatauil aatauil commented Feb 12, 2025

ID

DL-6240

Description

Currently it is possible to upload files that are 0 bytes. This does not seem very logical. We want to have an option to define a minimum file size and show an error if the file size is lower then that.

Discussion:

  • Do we want to instead make it "minFileSizeKB" and not be consistent with maxFile?
  • Do we just create a option that says, "notEmpty" and put the default to 1kb?

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Maintanance

How to setup

No special setup requirements

How to test

Set the minFileSizeMB to "0.01" (10kb). Try to upload an empty file, you should see the error message. While

@Windvis
Copy link
Contributor

Windvis commented Feb 13, 2025

Should this even be an option? Uploading an empty file does indeed not make sense and do we ever want to configure the minimum size (besides larger than 0)?

@Windvis Windvis added the enhancement Used when the PR adds a new feature or enhancement. label Feb 13, 2025
@aatauil
Copy link
Contributor Author

aatauil commented Feb 13, 2025

@Windvis I guess this option is just more flexible towards the future. I can think of a usecase where a pdf template gets used and that template is by default 100kb, then just checking for 0 is not the solution to check for empty documents.

Update: I have modified the code to use KB instead of MB. setting 0.001mb for 1kb is just bad UX. Theirfor for MIN, kb makes more sense

@Windvis Windvis changed the title Feature/min size file upload Add a @minFileSizeKB argument to the <AuFileUpload> component Feb 13, 2025
@Windvis
Copy link
Contributor

Windvis commented Feb 13, 2025

I guess this option is just more flexible towards the future. I can think of a usecase where a pdf template gets used and that template is by default 100kb, then just checking for 0 is not the solution to check for empty documents.

Sure it's more flexible, but chances are high it will never be used. Anyway, I don't have a strong opinion about this, so if you think this is the way to go 👍.

@aatauil
Copy link
Contributor Author

aatauil commented Feb 13, 2025

@Windvis Its just an example, another I can think of is a situation where an empty pdf exported from acrobat contains some metadata making the filesize like 15 bytes which would not trigger the "Leeg bestand" error, a dev could then set it to 1kb as a sane minimum

@Windvis
Copy link
Contributor

Windvis commented Feb 13, 2025

@aatauil I just don't think that setting a minimum file size is the best way to "prevent" those types of issues. It feels very frail, but since it's optional it can't hurt having it either I guess.

@Windvis
Copy link
Contributor

Windvis commented Feb 13, 2025

Looks good 👍 Could you squash merge it? This can all be 1 commit, imo.

I can do it as well, but then Github adds me as a co-author I think 😄.

@Windvis Windvis merged commit b89717f into master Feb 13, 2025
10 checks passed
@Windvis Windvis deleted the feature/min-size-file-upload branch February 13, 2025 16:31
@Windvis
Copy link
Contributor

Windvis commented Feb 13, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Used when the PR adds a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants