Details

Reviewers

tomek
atul
ginsu
kamil
bartek
ashoat

Commits

rCOMM4d7c49a1d0ce: [native] Add community drawer to navigation hierarchy

Summary

Linear issue: https://linear.app/comm/issue/ENG-1881/community-navigation-drawer-on-native and https://linear.app/comm/issue/ENG-2327/change-code-around-getstatefromnavigatorroute-to-address-adding-drawer
Adding the drawer navigator between App and Tab navigators so it opens over the tabs. Some calls to getStateFromNavigatorRoute had to be changed as they assumed the navigation hierarchy. The dafault
state had to be updated to relfect the new hierarchy.

Test Plan

Checked the changes to activeThread by logging the value it returns, and the changes to baseCreateActiveTabSelector by logging the value returnes in ProfileHeader depending on the navigation state.
Run the ios simulator and checked that the drawer can be opened from the TabNavigator header and the drawer behaves as expected: CommunityDrawerContent is displayed, navigation to threads works, etc.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

inka created this revision.Nov 28 2022, 6:09 AM

Herald added a subscriber: ashoat. · View Herald TranscriptNov 28 2022, 6:09 AM

Harbormaster completed remote builds in B13778: Diff 18864.Nov 28 2022, 6:23 AM

inka requested review of this revision.Nov 28 2022, 6:23 AM

getStateFromNavigatorRoute is called from nav-selectors.js: baseCreateActiveTabSelector, scrollBlockingModalsClosedSelector, selectBackgroundIsDark and activeThread. scrollBlockingModalsClosedSelector and selectBackgroundIsDark look at scrollBlockingModals that are children of App component.TabNavigator is not one of them, and neither is DrawerNavigator so the changes don't influence the behaviour.
The code in the other two functions has been changed and tested as described in the revision.

getStateFromNavigatorRoute is called from navigation-utils.js: currentRouteRecurse, getChildRouteFromNavigatorRoute. None of these require any changes.

getStateFromNavigatorRoute is called from thread-screen-pruner.react.js: https://github.com/CommE2E/comm/blob/master/native/chat/thread-screen-pruner.react.js#L52. The code around this has been changed in this diff.

inka edited the summary of this revision. (Show Details)Nov 28 2022, 6:35 AM

Great work spotting all of these places that need to be updated!!

That said, I'm pretty sure TabNavigator needs to be updated now since its types are probably wrong now

This revision now requires changes to proceed.Nov 28 2022, 9:56 AM

inka mentioned this in D5732: [native] Add community drawer navigator.Nov 29 2022, 5:44 AM

Fix TabNavigator typing

inka added inline comments.Nov 29 2022, 5:49 AM

native/navigation/tab-navigator.react.js
70 ↗	(On Diff #18946)	I believe this was incorrect in the first place, and that was my fault.

Harbormaster completed remote builds in B13834: Diff 18946.Nov 29 2022, 6:01 AM

ashoat added inline comments.Nov 29 2022, 8:35 AM

native/navigation/tab-navigator.react.js
70 ↗	(On Diff #18946)	Good call – should've caught this in review!

Would love if @tomek could take a look as well (or at least one other person)

native/navigation/community-drawer-navigator.react.js
46 ↗	(On Diff #18946)	This should be in a `React.useMemo`

tomek accepted this revision.Nov 30 2022, 8:35 AM

tomek added inline comments.

native/navigation/nav-selectors.js
162–172 ↗	(On Diff #18946)	It seems like we're duplicating most of this logic. It might be a good idea to refactor it later by extracting a function.