-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 gif support to web #6433
Add gif support to web #6433
Conversation
if (uri.startsWith('data:video/')) { | ||
onSelectVideo(post.id, {uri, type: 'video', height: 0, width: 0}) | ||
if (uri.startsWith('data:video/') || uri.startsWith('data:image/gif')) { | ||
if (isNative) return // web only |
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.
is it possible to paste videos on native? do we need to handle this? it will be broken in prod anyway since the old code sets the height/width to 0
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.
it seems that pasting video is not supported, but pasting gifs is supported. https://github.com/mattermost/react-native-paste-input/blob/master/ios/UIPasteboard%2BGetImageInfo.m#L36
of course, since we are not going to add support for uploading gifs on native quite yet, this isnt something to worry about for now unless you feel so inclined (fwiw, i think it would work as is if we allowed pasting them no?)
|
This comment has been minimized.
This comment has been minimized.
|
I sincerely apologize for misunderstood the meaning. |
All good, don't worry about it |
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.
looks fab lets go
a7435fe
to
0f4323b
Compare
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.
ok but needs cross browser testing
tested cross browser (ff, chrome, safari, mobile safari) |
}) | ||
} | ||
img.onerror = (_ev, _source, _lineno, _colno, error) => { | ||
console.log('Failed to grab GIF metadata', error) |
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.
Any reason why console.log is left in here?
Allows adding gifs on web - processed as video.
Also reimplements the video picker, since
expo-image-picker
's implementation on web is absolutely terrible, does not support half the props, and does not support changing the accepted mime types. This change greatly simplifies the logic for normal videos too, since the preview no longer needs to pull double duty detecting the image dimensions or duration - this has been moved togetVideoMetadata
which is called when the video is first picked. This can and should still be simplified further, since we're inefficiently passing dataurls around, but this is easy for now since it lets us maintain compatibility with theImagePickerAsset
typeNote
I don't have a way of detecting how long a gif is (i.e. it could be not even animated, it could be >60s) so we just have to let the server reject these. Apparently it seems you can parse the bytes manually to determine this? Hopefully that's uncommon enough that we can get away with not rejecting on the client
TODO (but out of scope for this PR): different presentation for gifs. needs lexicon change.
Screen.Recording.2024-11-16.at.21.51.39.mov
Test plan