- Notifications
You must be signed in to change notification settings - Fork583
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(ui): add max. file size check before upload attachment#2709
Conversation
|
✅ Deploy Preview forelk-docs canceled.
|
✅ Deploy Preview forelk-zone ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
It seems
|
I found this report on Mastodon repository:mastodon/mastodon#22757 I suspect 413 responses are raised due to the HTTP server application (or some kind of proxy?) setting, instead of Mastodon setting. It looks like if Mastodon checks the uploaded video, it will return 422 error when the size is exceeded. If this assumption is correct, we will be able to upload 16MB video by adjusting the server configuration. Either way, this new warning is a good addition. Edit: this is the same advice to increase |
@shuuji3 the problem is that masto api not throwing proper error and so we cannot access response status code, 413 should be used since it is the proper http response for it: entity too large iirc. This PR should be ready, just added audio and unknown logic and messages. |
should we use |
Intl.NumberFormat working also in FireFox, we need to test it in Safari:
|
checked quickly on Safari console (macOS) and it works as expected. This method is well supported now so it should be ok for all modern browsers:https://caniuse.com/?search=Intl.NumberFormat
Does the masto.js interface has changed a lot? If we need to use the API differently, maybe it would be better to a separate PR to isolate the change. Otherwise, I think we can switch to v2. |
The media v2 no, only the create method (now sent with a multipart form), maybe also the implementation the api is the same. |
We alos need to handle kb, I'm going to move the logic to i18n composable. |
Nice |
The I tried uploading 10,000x10,000px png image (exceeding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I tested large audio files (90MB and 100MB) on mastodon.social, which configured 100MB as the video size limitation. I couldn't find the audio specific size limitation but the limitation is virtually the same as the video size limitation (<100MB). So it's reasonable to assume the limit of the audio size is the same as video.
Co-authored-by: TAKAHASHI Shuuji <shuuji3@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks
…#2709)Co-authored-by: TAKAHASHI Shuuji <shuuji3@gmail.com>
This PR prevents the file upload using the
mediaAttachmentsinstance configuration:videoSizeLimitfor videosimageSizeLimitfor imagesThere are 2 more entries
imageMatrixLimitandvideoMatrixLimit, no idea if we should use them.There is a problem with audio files, for example, webtoo.ls allowing audio files but there is no
audioSizeLimitinmediaAttachmentsoptions: should we use video or image? Right now using video size limit (we also need to include the audio message, this PR using the video message).This PR also includes 2 new entries in
state, using Intl.NumberFormat insideuseUploadMediaAttachmentcomposable (composables/masto/publish.ts module):attachments_limit_image_errorattachments_limit_video_errorattachments_limit_audio_errorattachments_limit_unknown_errorThis PR includes translations for English (en.json), Spanish (es.json and es-419.json): also fixes an entry in es.json (video => vídeo). Messages are not reactive: if you go to the settings and change the locale the messages in the compose page will not be updated.
/cc@mrcego
closes#1674
en.json
es.json
es-419.json