[web] Refactor Mutlimedia to function component
ClosedPublic
Actions

Authored by bartek on May 21 2023, 7:53 AM.

Details

Reviewers

ashoat
atul
ginsu
inka

Commits

rCOMMc6c65354ee64: [web] Refactor Mutlimedia to function component

Summary

Refactor the <Multimedia> component to be a function component. This will make managing further thumbhash logic much easier.

Test Plan

Multimedia are still displayed as before (both in chat and input bar). Clicking on the message opens modal. Remove button in input bar works. URL revoking is tested by console.log and ensuring it's called once when the message is sent.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.May 21 2023, 7:53 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptMay 21 2023, 7:53 AM

bartek added a child revision: D7900: [web] Add thumbhash to encrypted media, fix spinner.May 21 2023, 7:56 AM

bartek published this revision for review.May 21 2023, 8:07 AM

Harbormaster completed remote builds in B19545: Diff 26726.May 21 2023, 8:11 AM

ashoat accepted this revision.May 22 2023, 8:13 AM

ashoat added inline comments.

web/media/multimedia.react.js
84–96 ↗	(On Diff #26726)	It appears that this code has changed... previously it was just passing `mediaSource` to `media`. Two quick notes: Can you clarify why it changed? Was it a Flow issue? When possible, it's always best to separate changes from refactor diffs... that way it's easier for the reviewer to "blindly accept" the refactor, and then pay closer attention to the actual changes
93 ↗	(On Diff #26726)	Wondering why this `invariant` is necessary... based on Flow types, it appears that `uri` should always be set (but perhaps it can be an empty string?)
155 ↗	(On Diff #26726)	Wondering why you replaced the spread with explicit props. Is it for readability? Or does a future diff add a prop or something?
191 ↗	(On Diff #26726)	This might be a good component to wrap with a `React.memo`... I assume the performance costs of rerendering media are non-negligible

This revision is now accepted and ready to land.May 22 2023, 8:13 AM

ashoat added inline comments.May 22 2023, 9:50 AM

web/media/multimedia.react.js
155 ↗	(On Diff #26726)	This becomes more clear in D7902. It would have been easier to review if this diff left the spread as-is, since we only needed to remove the spread in D7902 where we start passing different props

bartek added inline comments.May 23 2023, 7:46 AM

web/media/multimedia.react.js
84–96 ↗	(On Diff #26726)	The logic stays the same, it just makes better ground for future diffs, but you're right this could have been possibly done in one of the next diffs. This is reverted anyway in D7947 When possible, it's always best to separate changes from refactor diffs... that way it's easier for the reviewer to "blindly accept" the refactor, and then pay closer attention to the actual changes Ok, makes sense
93 ↗	(On Diff #26726)	Wondering why this `invariant` is necessary... based on Flow types, it appears that `uri` should always be set (but perhaps it can be an empty string?) This invariant is for Flow which for some reason isn't able to see that we're already excluded `EncryptedMediaType` media sources, and for `MediaType` sources `uri` is always present. Anyway, I update the `MultimediaModal` props to match `mediaSource` type again so this change will be reverted.