Page MenuHomePhabricator

[native] Allow user to ‘see more’ on native and navigate to a full screen gallery
ClosedPublic

Authored by rohan on Feb 7 2023, 6:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:01 PM
Unknown Object (File)
Sat, Nov 23, 8:57 PM
Unknown Object (File)
Sat, Nov 23, 8:27 PM
Unknown Object (File)
Sat, Nov 23, 8:21 PM
Unknown Object (File)
Sat, Nov 23, 8:10 PM
Unknown Object (File)
Sat, Nov 23, 7:41 PM
Unknown Object (File)
Tue, Nov 19, 12:55 AM
Unknown Object (File)
Mon, Nov 18, 10:09 AM
Subscribers

Details

Summary

Following the designs (https://linear.app/comm/issue/DES-27/view-media-for-chat), users should be able to click "see more" to navigate from the thread settings media gallery to a full screen media gallery. Using ThreadSettingsCategoryHeader didn't allow for a title and also additional text ("see more"), so I made another component that takes an onPress and actionText. This way, we can navigate to the full screen gallery when actionText is pressed. The next diff will implement the actual full screen gallery.

Linear:
https://linear.app/comm/issue/ENG-2876/allow-user-to-see-more-on-native-and-navigate-to-a-full-screen-gallery

Depends on D6659

Test Plan

Videos for both iOS and Android will be shown below.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan requested review of this revision.Feb 7 2023, 7:01 PM
native/chat/chat.react.js
255 ↗(On Diff #22257)

I can't tell from the designs if this is supposed to be "Media Gallery" or "All Media"

All wondering about the capitalization, seems inconsistent with the rest of the app but maybe it makes sense here?

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

What are these spaces doing here? I'm pretty sure they get ignored

native/chat/settings/thread-settings-category.react.js
43 ↗(On Diff #22257)
  1. What makes an "action header" different from a header? Should this be merged with the main header?
  2. Why are we bothering to support all these categoryTypes if we're not actually using them?
native/chat/chat.react.js
255 ↗(On Diff #22257)

Ah you're right, I missed that it changes from "Media Gallery" in the thread settings to "All Media" in the full screen gallery.

I did notice that it's inconsistent, so we could change the design here to make it "All media". I prefer "All Media" but I don't have a strong opinion either way

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

Yeah you're right, I'll remove the spaces in the revision and <Text>{id}</Text> will be refactored out for the actual full screen media gallery

native/chat/settings/thread-settings-category.react.js
43 ↗(On Diff #22257)

The difference between an action header from the existing header is the actionText and onPress. The current headers have no need to support additional behavior (like text that will trigger an onPress). With the media gallery, we have the title "Media Gallery", but based on the designs we'd also need a second text component that, onPress, will navigate to the full screen gallery.

I think we could merge the two headers, it would mean making the default header accept two optional parameters actionText and onPress, and if so, conditionally render what's below, and modify the styles.header to include flexDirection: 'row',.

<TouchableOpacity onPress={props.onPress}>
          <Text style={styles.actionText}>{props.actionText}</Text>
</TouchableOpacity>

As for the three categoryType, I'm only using outline for the media gallery, but since I made the "action header" into it's own component, I thought it'd be best to support the same categoryTypes that the other two components here support, in case designs change or we want to include another ThreadSettingsCategoryActionHeader, but instead use unpadded for example like some of the other headers in the settings do

native/chat/chat.react.js
255 ↗(On Diff #22257)

Can you check with Ted?

native/chat/settings/thread-settings-category.react.js
43 ↗(On Diff #22257)

Got it – let's remove the unused params (including categoryType) from your new component. We can always add them later if we need them

Remove the unused categoryTypes from ThreadSettingsCategoryActionHeader, fix spacing in full screen gallery, and rename "Media Gallery" to "All Media" to follow designs (confirmed with Ted)

native/chat/settings/thread-settings-category.react.js
40

It's usually better to type callbacks where we don't care about the return type as mixed because it is more convenient

Agree with @tomek's nit, but otherwise this looks good

This revision is now accepted and ready to land.Feb 15 2023, 7:30 AM

Change type from void to mixed