feat: add tab navigator #5027

MuckT · 2024-03-04T23:15:00Z

Description

Adds the tab navigator and associated icons. Behavior is enabled with the use_tab_navigator feature gate. Please disregard the header display as this will be covered in ACT-1108.

Android	iOS 15 Pro Max - Large Font	iOS SE (3rd Generation)

Test plan

Tested locally on iOS with feature flag enabled and disabled
Tested locally on Android with feature flag enabled and disabled
Unit tests updated

Related issues

Fixes ACT-1103

Backwards compatibility

Yes - behind the use_tab_navigator feature gate.

Network scalability

N/A

emerge-tools · 2024-03-04T23:24:02Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
Celo (test) org.celo.mobile.test	1.80.0 (145)	24.2 MB	⬆️ 16.6 kB (0.07%)	60.3 MB	⬆️ 32.8 kB (0.05%)	N/A

Celo (test) 1.80.0 (145)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 32.8 kB (0.05%)
Total download size change: ⬆️ 16.6 kB (0.07%)

Largest size changes

Item	Install Size Change
main.jsbundle	⬆️ 32.8 kB

🛸 Powered by Emerge Tools

codecov · 2024-03-04T23:43:51Z

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 85.55%. Comparing base (df00a4d) to head (fae031b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5027      +/-   ##
==========================================
- Coverage   85.55%   85.55%   -0.01%     
==========================================
  Files         723      723              
  Lines       29448    29460      +12     
  Branches     5078     5082       +4     
==========================================
+ Hits        25195    25205      +10     
- Misses       4020     4022       +2     
  Partials      233      233

Files	Coverage Δ
src/navigator/Screens.tsx	`100.00% <100.00%> (ø)`
src/navigator/initialRoute.tsx	`97.36% <100.00%> (+0.22%)`	⬆️
src/statsig/constants.ts	`100.00% <ø> (ø)`
src/statsig/types.ts	`100.00% <100.00%> (ø)`
src/tokens/Assets.tsx	`81.90% <ø> (ø)`
src/navigator/NavigationService.ts	`23.28% <50.00%> (+0.75%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df00a4d...fae031b. Read the comment docs.

MuckT · 2024-03-05T01:50:19Z

src/navigator/NavigationService.ts

@@ -238,7 +240,9 @@ export function navigateHome(options?: NavigateHomeOptions) {
  setTimeout(() => {
    navigationRef.current?.reset({
      index: 0,
-      routes: [{ name: Screens.DrawerNavigator, params }],
+      routes: getFeatureGate(StatsigFeatureGates.USE_TAB_NAVIGATOR)


On Android I am getting the error: The action 'RESET' with payload {"index":0,"routes":[{"name":"TabNavigator"}]} was not handled by any navigator. likely form here. When testing the non tab navigator path no error is served. On iOS I have not observed this same issue. Perhaps this should be fixed the follow up ACT-1110.

i'm confused by your PR comment and the comment in the code: the code says there's a TODO to handle undefined/falsey, the code seems to handle it by reverting to previous behavior (DrawerNavigator), and you PR comment seems to imply that addresses act-1110 will solve a related (unrelated?) problem?

Act-1110 is the feature gate ticket. As this relates to the feature gate not being defined when the navigation service is initialized, we try to initialize Statsig as early as possible, it seems appropriate to lump it into that work.

The comment seems misleading, it doesn't say anything about the error you ran into. It seems like we need to handle undefined return value from the gate, which is already handled. Any false-y value just picks the DrawerNavigator screen.

# Conflicts: # package.json # yarn.lock

silasbw · 2024-03-05T20:21:54Z

src/navigator/TabNavigator.tsx

+      <Tab.Screen
+        // TODO(act-1104) new assets screen
+        name={Screens.Assets}
+        // @ts-expect-error Type '{}' is missing the following properties from type 'Props': navigation, route


can we avoid using @ts-expect-error here? I worry (in general) that will forget about suppressed errors and they eventually surface as errors to the user experience.

This is just linking to the existing assets screen, but that screen be redone in act-1104 which should resolve this todo.

silasbw · 2024-03-05T20:24:52Z

src/navigator/NavigationService.ts

@@ -238,7 +240,9 @@ export function navigateHome(options?: NavigateHomeOptions) {
  setTimeout(() => {
    navigationRef.current?.reset({
      index: 0,
-      routes: [{ name: Screens.DrawerNavigator, params }],
+      routes: getFeatureGate(StatsigFeatureGates.USE_TAB_NAVIGATOR)


i'm confused by your PR comment and the comment in the code: the code says there's a TODO to handle undefined/falsey, the code seems to handle it by reverting to previous behavior (DrawerNavigator), and you PR comment seems to imply that addresses act-1110 will solve a related (unrelated?) problem?

satish-ravi

The padding on the tab bar looks off compared to figma

src/icons/AccountCircle.tsx

locales/base/translation.json

src/navigator/DrawerTopBar.tsx

src/navigator/Navigator.tsx

src/navigator/types.tsx

This reverts commit 1daa4b5.

MuckT · 2024-03-05T23:55:50Z

@satish-ravi added some padding in to better match styles in c5bc02d. It's a bit of a compromise as getting it pixel perfect on one screen can lead to some strange display on smaller screens; I've updated the screenshots in the description.

src/navigator/types.tsx

satish-ravi · 2024-03-06T00:53:06Z

src/navigator/NavigationService.ts

@@ -238,7 +240,9 @@ export function navigateHome(options?: NavigateHomeOptions) {
  setTimeout(() => {
    navigationRef.current?.reset({
      index: 0,
-      routes: [{ name: Screens.DrawerNavigator, params }],
+      routes: getFeatureGate(StatsigFeatureGates.USE_TAB_NAVIGATOR)


The comment seems misleading, it doesn't say anything about the error you ran into. It seems like we need to handle undefined return value from the gate, which is already handled. Any false-y value just picks the DrawerNavigator screen.

satish-ravi · 2024-03-06T00:57:05Z

src/navigator/TabNavigator.tsx

+    >
+      <Tab.Screen
+        // TODO(act-1104) new assets screen
+        name={Screens.Assets}


Is there any risk of collision with the actual Assets/Home/Dapps screens here? i.e., is there a possibility that any component that says navigate(Screens.Assets) navigates to this Tab screen instead of the original Assets screen while this feature is off? Maybe we should just use unique names to be safe? We can fill in with the actual tab screens in the follow up tickets

I think it's fine to leave these as navigating as is. I've tested the navigation when StatsigFeatureGates.USE_TAB_NAVIGATOR is false and the existing navigation is tested in the e2e tests.

satish-ravi · 2024-03-06T01:16:09Z

src/navigator/NavigationService.ts

@@ -224,7 +226,7 @@ export async function isBottomSheetVisible(screen: Screens) {
 }

 interface NavigateHomeOptions {
-  params?: StackParamList[Screens.DrawerNavigator]
+  params?: StackParamList[Screens.DrawerNavigator] | StackParamList[Screens.TabNavigator]


This type is kind of a problem, as it masks a potential issue. The initialScreenName accepted by the two screens are actually going to be different. What if a caller passes an initialScreenName that only applies to the DrawerNavigator. E.g., we have a the CYA screen passing DAppsExplorerScreen and ExchangeHomeScreen, would it be on the caller to also get the gate and pass the appropriate screen name? I wonder if we should make the typing more strict so we can catch these potential issues that may be hidden.

It looks like react-navigation handles internally. I've tested navigating from the CYA screen to each option and the correct screen is served for both paths for the use_tab_navigator feature gate.

guess this might be broken now with changing the screen names, but we can fix it in ACT-1110

# Conflicts: # src/navigator/Screens.tsx # test/RootStateSchema.json

src/navigator/TabNavigator.tsx

satish-ravi · 2024-03-08T20:21:53Z

src/navigator/NavigationService.ts

@@ -224,7 +226,7 @@ export async function isBottomSheetVisible(screen: Screens) {
 }

 interface NavigateHomeOptions {
-  params?: StackParamList[Screens.DrawerNavigator]
+  params?: StackParamList[Screens.DrawerNavigator] | StackParamList[Screens.TabNavigator]


guess this might be broken now with changing the screen names, but we can fix it in ACT-1110

Co-authored-by: Satish Ravi <satish.ravi@valoraapp.com>

src/navigator/TabNavigator.tsx

Co-authored-by: Satish Ravi <satish.ravi@valoraapp.com>

### Description Adds the tab navigator and associated icons. Behavior is enabled with the `use_tab_navigator` feature gate. Please disregard the header display as this will be covered in ACT-1108. | Android | iOS 15 Pro Max - Large Font | iOS SE (3rd Generation) | | ----- | ----- | ----- | | ![](https://github.com/valora-inc/wallet/assets/26950305/e65dbbd2-c449-4b17-8b7b-904bfa3463f9 "Android Assets") | ![](https://github.com/valora-inc/wallet/assets/26950305/d74ae00e-4ae8-4ae2-9a4b-bb54377da375 "iOS 15 Pro Max Assets") | ![](https://github.com/valora-inc/wallet/assets/26950305/ee815632-29ee-46de-9145-8a9c420ac01a "iOS SE (3rd Generation) Assets") | ### Test plan - [x] Tested locally on iOS with feature flag enabled and disabled - [x] Tested locally on Android with feature flag enabled and disabled - [x] Unit tests updated ### Related issues - Fixes ACT-1103 ### Backwards compatibility Yes - behind the `use_tab_navigator` feature gate. ### Network scalability N/A --------- Co-authored-by: Satish Ravi <satish.ravi@valoraapp.com>

MuckT added 5 commits March 4, 2024 14:40

build(deps): add @react-navigation/bottom-tabs

2c974a2

feat: add tab navigator and icons

908d147

chore: add use tab navigator feature gate

8baa126

feat: use tab navigator feature gate

d558d07

style: use ts-expect-error with description of error

03db3ca

MuckT added 2 commits March 4, 2024 15:33

test: update initial route tests for use tab navigator feature gate

1c6b0fc

chore: update root state schema

6741500

MuckT marked this pull request as ready for review March 5, 2024 00:50

MuckT added 2 commits March 4, 2024 16:54

docs: add relevant todos

433fd92

fix: remove unused icon prop

339fc34

MuckT commented Mar 5, 2024

View reviewed changes

MuckT added 3 commits March 4, 2024 18:02

docs: add todo for navigation reset

06cc5e2

fix: label styles for tab bar text

533ce36

Merge branch 'main' into tomm/act-1103

a8fa47a

# Conflicts: # package.json # yarn.lock

silasbw reviewed Mar 5, 2024

View reviewed changes

satish-ravi reviewed Mar 5, 2024

View reviewed changes

MuckT and others added 9 commits March 5, 2024 14:08

docs: add todos for header related ticket

d7ba169

fix: register both navigators

5e5a279

chore: remove unused screen prop

1daa4b5

chore: use more descriptive translation key

d428981

Revert "chore: remove unused screen prop"

34a5d68

This reverts commit 1daa4b5.

fix: add view box to icons

370ffa7

Merge branch 'main' into tomm/act-1103

2d4f675

fix: add padding to top of tab bar item to better match styles

c5bc02d

fix: disallow font scaling in bottom tab navigator

af8e557

MuckT added 2 commits March 5, 2024 16:28

chore: remove changes to drawer top bar and use tab navigator header

4e6695f

chore: prevent header title font scaling

6f6d572

docs: use beta tag for unused icon

e8b946b

satish-ravi reviewed Mar 6, 2024

View reviewed changes

MuckT and others added 8 commits March 5, 2024 17:50

chore: remove unused file

2589e10

docs: remove unneeded todo comment

1b4d38a

Merge branch 'main' into tomm/act-1103

6583eb5

# Conflicts: # src/navigator/Screens.tsx # test/RootStateSchema.json

fix: use variable height for tab bar navigator

c013c4b

chore: update tab bar label and icon

1fe8c97

chore: use new tab screen names

55abbca

Merge branch 'main' into tomm/act-1103

ebc90f1

chore: update root state schema

e6a6858

satish-ravi approved these changes Mar 8, 2024

View reviewed changes

Update src/navigator/TabNavigator.tsx

77730db

Co-authored-by: Satish Ravi <satish.ravi@valoraapp.com>

satish-ravi reviewed Mar 8, 2024

View reviewed changes

src/navigator/TabNavigator.tsx Outdated Show resolved Hide resolved

Update src/navigator/TabNavigator.tsx

fae031b

Co-authored-by: Satish Ravi <satish.ravi@valoraapp.com>

MuckT added this pull request to the merge queue Mar 8, 2024

Merged via the queue into main with commit d2ae03d Mar 8, 2024
16 checks passed

MuckT deleted the tomm/act-1103 branch March 8, 2024 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tab navigator #5027

feat: add tab navigator #5027

MuckT commented Mar 4, 2024 •

edited

emerge-tools bot commented Mar 4, 2024 •

edited

codecov bot commented Mar 4, 2024 •

edited

MuckT Mar 5, 2024

silasbw Mar 5, 2024

MuckT Mar 5, 2024 •

edited

satish-ravi Mar 6, 2024

silasbw Mar 5, 2024

MuckT Mar 5, 2024

silasbw Mar 5, 2024

satish-ravi left a comment

MuckT commented Mar 5, 2024

satish-ravi Mar 6, 2024

satish-ravi Mar 6, 2024

MuckT Mar 6, 2024

satish-ravi Mar 6, 2024 •

edited

MuckT Mar 8, 2024

satish-ravi Mar 8, 2024

satish-ravi Mar 8, 2024

feat: add tab navigator #5027

feat: add tab navigator #5027

Conversation

MuckT commented Mar 4, 2024 • edited

Description

Test plan

Related issues

Backwards compatibility

Network scalability

emerge-tools bot commented Mar 4, 2024 • edited

1 build increased size

Celo (test) 1.80.0 (145)org.celo.mobile.test

codecov bot commented Mar 4, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuckT Mar 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satish-ravi left a comment

Choose a reason for hiding this comment

MuckT commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satish-ravi Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuckT commented Mar 4, 2024 •

edited

emerge-tools bot commented Mar 4, 2024 •

edited

Celo (test) 1.80.0 (145)
org.celo.mobile.test

codecov bot commented Mar 4, 2024 •

edited

MuckT Mar 5, 2024 •

edited

satish-ravi Mar 6, 2024 •

edited