Page MenuHomePhabricator

[native] community joiner modal
Needs RevisionPublic

Authored by varun on Nov 21 2024, 7:58 PM.
Tags
None
Referenced Files
F3514828: D13996.id46372.diff
Sun, Dec 22, 6:39 AM
F3514827: D13996.id45933.diff
Sun, Dec 22, 6:39 AM
F3514807: D13996.id.diff
Sun, Dec 22, 6:39 AM
F3514799: D13996.diff
Sun, Dec 22, 6:38 AM
F3510545: D13996.diff
Sat, Dec 21, 12:03 PM
Unknown Object (File)
Wed, Dec 18, 2:12 PM
Unknown Object (File)
Fri, Dec 13, 9:21 PM
Unknown Object (File)
Fri, Dec 13, 4:58 AM
Subscribers

Details

Reviewers
ashoat
Summary

Depends on D13995

we'll navigate to this modal from the community drawer button in D13987 and from the bottom sheet in D13986

Test Plan

tested that the tabs work as expected and all the community threads display in the right tab. tested the join/leave behavior in previous diff, which also has a video with the modal

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/components/community-joiner-modal.react.js
134 ↗(On Diff #45933)

this is obviously wrong, but was having some trouble adding a libdef for react-native-tab-view. i couldn't figure out how to import the Animated type from 'react-native' in my libdef

figured I'd put this diff up before spending too much time on this, since @ashoat might have some ideas here

134 ↗(On Diff #45933)

by wrong, I mean not how we usually resolve flow issues

native/components/community-joiner-modal.react.js
19 ↗(On Diff #45933)

we don't want to display an empty modal so we make sure we have the list of communities first before we navigate to this modal

30 ↗(On Diff #45933)
varun requested review of this revision.Nov 21 2024, 8:18 PM

btw, this diff replaces D13493, which I plan on abandoning. i think i've applied all the feedback from there, but referencing here just in case

ashoat requested changes to this revision.Sat, Nov 23, 6:16 AM
ashoat added inline comments.
native/components/community-joiner-modal.react.js
46 ↗(On Diff #45933)

Is this the best way to check which communities belong where? What happens if a community gets renamed? I feel like IDs might be better

87–101 ↗(On Diff #45933)

Should these be in useCallbacks?

109–112 ↗(On Diff #45933)

Should this be in a useMemo?

134 ↗(On Diff #45933)

Talked through this offline yesterday, but yeah we should improve this from mixed

134–142 ↗(On Diff #45933)

Shouldn't this be in a useCallback?

150 ↗(On Diff #45933)

Shouldn't this be in a useMemo?

153 ↗(On Diff #45933)

Shouldn't this be in a useMemo?

This revision now requires changes to proceed.Sat, Nov 23, 6:16 AM
native/components/community-joiner-modal.react.js
46 ↗(On Diff #45933)

You're right. I wrote a script here that I'll need you to run to get the IDs

D14145

native/components/community-joiner-modal.react.js
35

This should say IDs instead of names, but the comment will be removed entirely when I populate the list

ashoat requested changes to this revision.Thu, Dec 12, 11:21 AM

This is close, but requesting changes due to the volume of comments below

native/components/community-joiner-modal.react.js
35

Probably best to put it in a separate file, as I imagine it'll be somewhat long

40

Why is this fallback necessary? CommunityJoinerModalParams seems to indicate that we'll always have params.communities

73

Can you remind me in what cases the keyserver returns a community with a missing threadInfo? Should we consider just not having the keyserver return those communities, or do we need them for something?

125–130

Nit: shorthand

152

Curious why we need this

Also, should this be defined at the top level instead of in a React.useMemo?

native/flow-typed/npm/react-native-tab-view_v3.x.x.js
6–9

This is cool! Didn't know about these types. How'd you find out about them?

39

Besides the missing +, this type doesn't seem right. The second part of the & says it is an object with precisely one property: route. If that's the case, then what's the point of including SceneRendererProps there?

Guessing you probably want ... as well:

+renderScene: (props: SceneRendererProps & { +route: Route<>, ... }) => React$Node,
48

Same feedback as above here (consider ...)

This revision now requires changes to proceed.Thu, Dec 12, 11:21 AM
native/components/community-joiner-modal.react.js
73

the keyserver should never return a community with a missing threadInfo, but it's technically possible. (we call fetchPrivilegedThreadInfos on the keyserver to get the threadInfos, and this function could return an empty array.) we can just omit the communities that are missing a threadInfo and maybe add a log on the keyserver to point out the unexpected state

152

passing initialLayout can improve the initial rendering performance. you're right that this should be at the top level since it's static

native/flow-typed/npm/react-native-tab-view_v3.x.x.js
6–9

I git grep'd for these types in the flow-typed subdirectory. but now that i look more closely, these are both any-typed in native/flow-typed/npm/react-native-gesture-handler_v2.x.x.js...

declare export type AnimatedValue = any;
declare export type ViewStyle = any;
native/flow-typed/npm/react-native-tab-view_v3.x.x.js
6–9

Maybe better to copy-paste the "Vaguely copied" lines definitions in native/flow-typed/npm/@react-navigation/core_v6.x.x.js

native/components/community-joiner-modal.react.js
152

Let's calculate this based on our styles. First we factor them out:

// @flow

export const modalBorderWidth = 2;

export const modalMarginHorizontal = 15;

export const modalPadding = 12;

Then we update native/components/modal.react.js to use those.

And finally we calculate this initialLayout width based on useWindowDimensions().width - 2 * (sum of above)