Page MenuHomePhabricator
Differential D5759

[web] Display videos correctly in `Multimedia` component
ClosedPublic
Actions

Authored by atul on Nov 29 2022, 1:13 PM.
Tags
None
Referenced Files
F3509300: D5759.diff
Sat, Dec 21, 3:15 AM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
Unknown Object (File)
Thu, Dec 19, 9:35 PM
Unknown Object (File)
Thu, Dec 19, 9:34 PM
Unknown Object (File)
Thu, Dec 19, 9:21 PM
Unknown Object (File)
Sat, Dec 14, 2:06 PM
Unknown Object (File)
Tue, Dec 10, 12:00 PM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
ginsu
rohan
ashoat
Commits
rCOMMd0a756e73375: [web] Display videos correctly in `Multimedia` component
Summary

Context: https://linear.app/comm/issue/ENG-1512/render-video-messages-inline-on-web

Basically display media with <video> tag instead of <img> tag if mediaType === "video". Also remove any onClick functionality since we don't have any sort of full-screen video playback experience on web.

Test Plan

Here's how it looks in Safari:

8edddc.png (1×1 px, 2 MB)

Here's how it looks in Chrome:

3e73f7.png (1×1 px, 2 MB)

Made sure that images continue to look as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul created this revision.Nov 29 2022, 1:13 PM
Herald added a subscriber: ashoat. · View Herald TranscriptNov 29 2022, 1:13 PM
atul published this revision for review.Nov 29 2022, 1:15 PM
atul edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B13855: Diff 18977.Nov 29 2022, 1:26 PM
ashoat accepted this revision.Nov 29 2022, 1:37 PM
ashoat added inline comments.
web/media/multimedia.react.js
22 ↗(On Diff #18977)

Should we use MediaType here? Will reduce the amount of refactoring we need to do if we introduce another one

This revision is now accepted and ready to land.Nov 29 2022, 1:37 PM
atul added inline comments.Nov 29 2022, 1:40 PM
web/media/multimedia.react.js
22 ↗(On Diff #18977)

Oh yeah definitely, will update

atul updated this revision to Diff 18978.Nov 29 2022, 2:02 PM

Type type prop of Multimedia as MediaType instead of 'photo' | 'video'

This revision was landed with ongoing or failed builds.Nov 29 2022, 2:14 PM
Closed by commit rCOMMd0a756e73375: [web] Display videos correctly in `Multimedia` component (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMMd0a756e73375: [web] Display videos correctly in `Multimedia` component.
Harbormaster completed remote builds in B13856: Diff 18978.Nov 29 2022, 2:15 PM

Revision Contents
Changeset List

Diff 18979

View Options

web/chat/chat-input-bar.react.js

Loading...
View Options

web/chat/multimedia-message.react.js

Loading...
View Options

web/media/media.css

Loading...
View Options

web/media/multimedia.react.js

Loading...