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

Limit video upload size #430

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Conversation

gordlin
Copy link
Member

@gordlin gordlin commented Nov 25, 2024

Related Item(s)

Issue #417

Changes

  • [FEATURE] Prevents videos larger than 75MB from being uploaded in video panels, and shows an error when attempting to upload them.

Testing

Steps:

  1. Go to any product (e.g. 00000000-0000-0000-0000-000000000000).
  2. Go to any video panel.
  3. Try to upload a video greater than 75MB. It will fail and show a warning.
  4. Try to upload a video less than 75MB. It will upload as expected.

This change is Reviewable

@gordlin gordlin added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Nov 25, 2024
Copy link

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/real-video-size-limit

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @gordlin)


src/components/video-editor.vue line 204 at r1 (raw file):

        // Block files which are too big
        if (file.size > this.fileSizeLimit * 1000000) {

This is nitpicky, but I believe this should be 1024 * 1024 instead of 1000000, otherwise we'll be capping it out at slightly less than 75MB.

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @gordlin and @RyanCoulsonCA)


src/components/video-editor.vue line 34 at r1 (raw file):

                <span class="align-middle inline-block">
                    <span>
                        <div>{{ $t('editor.video.label.drag') }}</div>

image.png

Center this first line by adding a text-align: center

@gordlin gordlin force-pushed the real-video-size-limit branch 2 times, most recently from f17fffc to 8602485 Compare December 5, 2024 20:21
Copy link
Member Author

@gordlin gordlin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)


src/components/video-editor.vue line 34 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

image.png

Center this first line by adding a text-align: center

Donethanks!


src/components/video-editor.vue line 204 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

This is nitpicky, but I believe this should be 1024 * 1024 instead of 1000000, otherwise we'll be capping it out at slightly less than 75MB.

Donethanks!

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

Copy link
Member

@szczz szczz left a comment

Choose a reason for hiding this comment

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

Just a temporary blocker as there are merge conflicts in lang.csv

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

@gordlin gordlin force-pushed the real-video-size-limit branch from 8602485 to 1e27582 Compare December 10, 2024 19:15
Copy link
Member Author

@gordlin gordlin left a comment

Choose a reason for hiding this comment

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

Donethanks!

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA, @szczz, and @yileifeng)

Copy link
Member

@szczz szczz left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

Copy link
Member

@IshavSohal IshavSohal left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

@yileifeng yileifeng merged commit ad21f78 into ramp4-pcar4:main Dec 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants