Details
- Reviewers
ashoat
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 |
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
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? |
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 |
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 ...) |
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) |