Details

Reviewers

atul
tomek
ginsu
ashoat

Commits

rCOMMea3f91b287d3: [native] Display both photos and videos in the media gallery on native

Summary

Now that the responders and endpoints are set up, we need to populate the FlatList with real data from the database. This means we need to get the data and use it as the data prop in the FlatList.

Previously, the component memoized an empty array mediaInfos to prevent the FlatList from re-rendering each time ThreadSettingsMediaGallery re-rendered, since otherwise the array would have a new reference. Now, with useState, the reference to mediaInfos should persist across re-renders and mediaInfos shouldn't change until we fetch the media. This will be important for when we incorporate a full screen media gallery and fetch more media as the user scrolls to populate mediaInfos.

https://linear.app/comm/issue/ENG-2875/display-both-photos-and-videos-in-the-media-gallery-on-native

Depends on D6587

Test Plan

Confirmed on both iOS and Android that the media loads in the thread settings. The videos are linked below.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Feb 7 2023, 5:08 PM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 7 2023, 5:08 PM

rohan edited the summary of this revision. (Show Details)Feb 7 2023, 5:09 PM

rohan edited the test plan for this revision. (Show Details)

rohan attached a referenced file: F369544: Screen Recording 2023-02-07 at 7.12.05 PM.mov. (Show Details)

Harbormaster completed remote builds in B16265: Diff 22256.Feb 7 2023, 6:14 PM

rohan requested review of this revision.Feb 7 2023, 6:14 PM

rohan edited the summary of this revision. (Show Details)Feb 7 2023, 6:45 PM

rohan added a child revision: D6660: [native] Allow user to ‘see more’ on native and navigate to a full screen gallery.

ashoat added inline comments.Feb 8 2023, 6:18 AM

native/chat/settings/thread-settings-media-gallery.react.js
63 ↗	(On Diff #22256)	Please avoid ternary in JSX like this

Remove ternary statement. Introduced renderMediaContainer to avoid copying code

Harbormaster completed remote builds in B16282: Diff 22279.Feb 8 2023, 8:33 AM

Remove ternary but remove renderMediaContainer actually, since it'll make it harder to potentially display timestamps on videos (I can bring it back when adding timestamps if it still reduces similar code)

Harbormaster completed remote builds in B16283: Diff 22280.Feb 8 2023, 8:55 AM

Rebase on the stack to have the updated stylings to match the design

Updated design:

Simulator Screen Shot - iPhone 14 Pro - 2023-02-09 at 11.32.09.png (2×1 px, 1 MB)

rohan attached a referenced file: F371434: Simulator Screen Shot - iPhone 14 Pro - 2023-02-09 at 11.32.09.png. (Show Details)Feb 9 2023, 8:33 AM

Harbormaster completed remote builds in B16301: Diff 22301.Feb 9 2023, 8:45 AM

Rebase on stack

Harbormaster completed remote builds in B16332: Diff 22334.Feb 10 2023, 12:47 PM

Rebase

Harbormaster failed remote builds in B16384: Diff 22421!Feb 13 2023, 6:48 AM

Rebase for CI failures

Harbormaster completed remote builds in B16393: Diff 22430.Feb 13 2023, 7:28 AM

tomek added inline comments.Feb 15 2023, 2:11 AM

native/chat/settings/thread-settings-media-gallery.react.js
82–93 ↗	(On Diff #22430)	Shouldn't we memoize `source` props?

ashoat requested changes to this revision.Feb 15 2023, 7:29 AM

ashoat added inline comments.

native/chat/settings/thread-settings-media-gallery.react.js
82–93 ↗	(On Diff #22430)	Again @rohan really want to emphasize that you need to get up to speed on memoization... otherwise it's going to continue causing repeated churn for you on every diff, and it's going to cause all of your reviewers to have to spend a lot more time on your diffs

This revision now requires changes to proceed.Feb 15 2023, 7:29 AM

rohan added inline comments.Feb 15 2023, 8:34 AM

native/chat/settings/thread-settings-media-gallery.react.js
82–93 ↗	(On Diff #22430)	Sorry... I considered it but looked through the codebase and thought (mistakenly) that I saw it wasn't a pattern to do so. I can make a memoized array that holds a uri for each index and use that array[index] in the `source` prop.

ashoat added inline comments.Feb 15 2023, 8:47 AM

native/chat/settings/thread-settings-media-gallery.react.js
82–93 ↗	(On Diff #22430)	Ah yeah we're not always perfect in the codebase today... Generally best to follow "first principles" on memoization and just ask yourself – will this lead to more render cycles if it's recomputed? If yes, always memoize (unless you'd be memoizing `children` prop or JSX, which is usually too messy to be worth it)

Memoize source props so we only recompute when mediaInfos changes

native/chat/settings/thread-settings-media-gallery.react.js
82–93 ↗	(On Diff #22430)	Got it, I'll be sure not to take the existing code as the only source of truth when making decisions

Looking better on memoization!

native/chat/settings/thread-settings-media-gallery.react.js
85 ↗	(On Diff #22627)	You're introducing the second direct usage of this in our codebase. That should give you pause... why don't we use this component directly in other places? You should think about whether there's another component in our codebase that you should be using. It's generally best to reuse internal components then to create new codepaths to external libraries
96 ↗	(On Diff #22627)	Shouldn't we just be showing the thumbnail?

This revision now requires changes to proceed.Feb 15 2023, 10:07 AM

Harbormaster completed remote builds in B16539: Diff 22627.Feb 15 2023, 10:09 AM

Wrote a comment in D6485, but in theory once that is done, we'd have a more "proper" Media item here that we can construct into a MediaInfo (by adding the index) and passing that as a prop into something like Multimedia. This allows us 1. not to display the videos on render but instead the thumbnails, and 2. Multimedia renders RemoteImage and that renders FastImage (I'll double check this once I get back to this diff though since I looked quickly)

Render Multimedia component to re-use internal components (this component already handles displaying the thumbnail for videos)

Harbormaster completed remote builds in B16590: Diff 22696.Feb 17 2023, 4:13 PM

Remove corners from MediaInfo here since we need to pass in a MediaInfo into Multimedia. Ran flow with no errors

rohan mentioned this in D6718: [native] Navigate to the image / video modal when a user clicks on a media item.Feb 17 2023, 4:39 PM

Harbormaster completed remote builds in B16591: Diff 22697.Feb 17 2023, 4:49 PM

Looks great, just a question

native/chat/settings/thread-settings-media-gallery.react.js
90 ↗	(On Diff #22697)	Can you explain why you added `GestureTouchableOpacity` here?

This revision now requires changes to proceed.Feb 20 2023, 2:32 PM

rohan added inline comments.Feb 20 2023, 2:48 PM

native/chat/settings/thread-settings-media-gallery.react.js
90 ↗	(On Diff #22697)	I didn't have a particularly strong reason, looking into `Multimedia` I saw the only other instance of it being rendered within some kind of touchable opacity was within the `GestureTouchableOpacity` here, originally introduced a while back. I know the same problem doesn't occur here but I decided to keep consistent. Happy to switch back to the react-native `TouchableOpacity` though

ashoat accepted this revision.Feb 20 2023, 3:11 PM

ashoat added inline comments.

native/chat/settings/thread-settings-media-gallery.react.js
90 ↗	(On Diff #22697)	Not asking about `GestureTouchableOpacity` vs `TouchableOpacity` I would've introduced the `GestureTouchableOpacity` in the same place where you set `onPress`, so it's clear what it's for