Page MenuHomePhabricator

[native] Create the full screen media gallery component
ClosedPublic

Authored by rohan on Feb 13 2023, 11:02 AM.
Tags
None
Referenced Files
F3336008: D6720.diff
Thu, Nov 21, 1:37 PM
Unknown Object (File)
Sun, Nov 10, 3:21 PM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 12:30 AM
Unknown Object (File)
Fri, Nov 8, 12:30 AM
Subscribers

Details

Summary

Here, we introduce the full screen media gallery on native that displays all of the media for the thread. This comes with rendering the ThreadSettingsMediaGallery with additional props, and also creating a FilterBar that is used to be able to filter out if we want to view all media, just photos, or just videos. As mentioned in the designs, we don't scroll back to the top automatically when a filter is selected, but instead we maintain the position.

Linear: https://linear.app/comm/issue/ENG-2929/create-the-full-screen-media-gallery-component-on-native

Depends on D6718

Test Plan

Check the full screen media gallery on the Android emulator, iOS emulator and my physical iPhone

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)
ashoat requested changes to this revision.Feb 13 2023, 4:01 PM
ashoat added inline comments.
native/chat/fullscreen-thread-media-gallery.react.js
38 ↗(On Diff #22491)

None of this inline-logic-inside-JSX stuff please... it hurts readability, hides control flow, and increases indentation

124 ↗(On Diff #22491)

Should this be wrapped in React.memo?

native/chat/settings/thread-settings-media-gallery.react.js
189–206 ↗(On Diff #22491)

Let's void .then syntax and always use await / async. If this breaks types, update them to accept any function that returns mixed (doesn't have to specifically return void)

native/chat/settings/thread-settings.react.js
1049 ↗(On Diff #22491)

Why are we passing our vertical bounds? Does the FullScreenThreadMediaGallery have the same vertical bounds? How are we going to maintain that property going forward? Should FullScreenThreadMediaGalleryRouteName just measure its own vertical bounds, since that's what we need for the ImageModal animation?

This revision now requires changes to proceed.Feb 13 2023, 4:01 PM

I'll address the feedback entirely in the next revision

native/chat/fullscreen-thread-media-gallery.react.js
38 ↗(On Diff #22491)

Got it, I'll remove it in the revision

native/chat/settings/thread-settings.react.js
1049 ↗(On Diff #22491)

I tried measuring it's own vertical bounds (pretty similar to how it's done here), and it seems like pageY remains the same but height is different. So it'll be best to change this and instead do the same thing and measure it's own vertical bounds.

We can then still pass them into ThreadSettingsMediaGallery as a prop

Remove .map() inside JSX, wrap component in React.memo, swap .then syntax to await / async, and calculate vertical bounds from within FullScreenThreadMediaGallery

native/chat/fullscreen-thread-media-gallery.react.js
46 ↗(On Diff #22608)

Would this be fine, or is there a preference to just do something like style={activeTab === Tabs.All ? styles.activeTabItem : styles.tabItem}?

ashoat requested changes to this revision.Feb 14 2023, 4:18 PM

Continues to be a lot of confusion about memoization

native/chat/fullscreen-thread-media-gallery.react.js
46 ↗(On Diff #22608)

It's a little unusual but fine imo. Only thing is that you don't need React.useCallback... you never pass the callback itself anywhere, and so there's no reason for it to be memoized

47 ↗(On Diff #22608)

On the other hand, THIS needs to be memoized

native/chat/settings/thread-settings-media-gallery.react.js
189 ↗(On Diff #22608)

We don't need to regenerate this function whenever offset changes. We only need to regenerate it if offset becomes undefined or stops being undefined. Can you update the dep list to reflect that, and reduce the amount of times onEndReached needs to be regenerated?

native/themes/colors.js
77 ↗(On Diff #22608)

We need to coordinate with the design team if we're adding a color – can you send the UX team a message about this?

This revision now requires changes to proceed.Feb 14 2023, 4:18 PM
rohan marked 2 inline comments as done.

Address feedback

native/themes/colors.js
77 ↗(On Diff #22608)

Spoke with Ted in person last week, the color matches the color from his designs on Figma and he said it's good for him

Can you send a message to the UX Team chat? I don’t think Ted is fully aware of Dave’s inflight work on colors.js. I don’t think Dave has colors for this new entry in his recent themes. cc @atul

ashoat requested changes to this revision.Feb 21 2023, 12:31 PM

Memoization confusion continues to abound. The current approach is simply not working... you can't continue to "guess and check". It's not a good use of my time and not a good use of your time.

Can you make sure that the next revision here doesn't have any memoization issues? If you need to, feel free to schedule a chat with me.

native/chat/fullscreen-thread-media-gallery.react.js
33

This literally does nothing

40

We shouldn't be memoizing the whole thing here

41–43
This revision now requires changes to proceed.Feb 21 2023, 12:31 PM

Can you make sure that the next revision here doesn't have any memoization issues? If you need to, feel free to schedule a chat with me.

Sure, I'll bring up my thoughts in our meeting tomorrow

This revision is now accepted and ready to land.Feb 22 2023, 1:51 PM