feat: add tab bar header navigation #5082

MuckT · 2024-03-13T02:20:10Z

Description

Adds the top navigation for tab navigation screens. In the screen shots below, the header is the main point of focus as different wallets were used to take enabled and disabled screenshots. Header icons were adjusted to allow for ripples on tap on Android and a few changes had to be made to keep alignment with the existing side DrawerTopBar.tsx.

Screenshots

Android Disabled	Android Enabled	iOS Disabled	iOS Enabled

Test plan

Tested locally on iOS
Tested locally on Android
Unit tests added

Related issues

Fixes #ACT-1108

Backwards compatibility

Yes

Network scalability

N/A

codecov · 2024-03-13T23:23:32Z

Codecov Report

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

Project coverage is 85.58%. Comparing base (409ace7) to head (f155a47).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5082      +/-   ##
==========================================
+ Coverage   85.55%   85.58%   +0.02%     
==========================================
  Files         725      730       +5     
  Lines       29754    29843      +89     
  Branches     5138     5148      +10     
==========================================
+ Hits        25457    25541      +84     
- Misses       4061     4066       +5     
  Partials      236      236

Files	Coverage Δ
src/analytics/Events.tsx	`100.00% <100.00%> (ø)`
src/analytics/Properties.tsx	`100.00% <ø> (ø)`
src/components/QrScanButton.tsx	`100.00% <100.00%> (ø)`
src/components/TokenBalance.tsx	`98.00% <ø> (ø)`
src/home/NotificationBell.tsx	`100.00% <100.00%> (ø)`
src/icons/AccountCircle.tsx	`100.00% <100.00%> (ø)`
src/navigator/Headers.tsx	`59.57% <100.00%> (+3.25%)`	⬆️
src/navigator/TabNavigator.tsx	`100.00% <100.00%> (ø)`
src/navigator/TopBarButton.tsx	`96.55% <ø> (ø)`
src/notifications/NotificationList.tsx	`100.00% <ø> (ø)`
... and 6 more

... and 4 files with indirect coverage changes

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 409ace7...f155a47. Read the comment docs.

MuckT · 2024-03-15T22:01:39Z

src/components/AccountCircleButton.tsx

+
+  return (
+    <View style={styles.container}>
+      <Touchable


Touchable used here and in other added icons instead of TopBarButton.tsx to better handle ripple on Android.

is this something TopBarIconButton can support? if not, is there a purpose of keeping TopBarIconButton around?

MuckT · 2024-03-15T22:03:01Z

src/escrow/EscrowedPaymentListScreen.tsx

-EscrowedPaymentListScreen.navigationOptions = titleWithBalanceNavigationOptions(
-  i18n.t('escrowedPaymentReminder')
-)
+EscrowedPaymentListScreen.navigationOptions = () => ({


Changed due to a failing test case in 2aeef88.

MuckT · 2024-03-15T22:05:50Z

src/home/WalletHome.tsx

@@ -96,6 +98,27 @@ function WalletHome({ route }: Props) {
    )
  }

+  // Scroll Aware Header


It might be worth abstracting this as we are now using the same pattern in two places.

MuckT · 2024-03-15T22:06:29Z

src/notifications/NotificationList.tsx

@@ -33,13 +31,6 @@ export function NotificationList<T>(props: Props<T>) {
  )
 }

-export function titleWithBalanceNavigationOptions(title: string) {


Unused after 2aeef88.

MuckT · 2024-03-15T22:07:11Z

src/tokens/Assets.tsx

@@ -205,7 +205,7 @@ function AssetsScreen({ navigation, route }: Props) {
  }

  return (
-    <>
+    <Animated.View>


A weird edge case, Android only, were an animated view wrapped in a fragment can cause transparency issues.

emerge-tools · 2024-03-18T16:00:19Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
⚠️ Celo (test) org.celo.mobile.test	1.80.0 (145)	26.5 MB	⬆️ 2.4 MB (9.82%)	63.1 MB	⬆️ 2.7 MB (4.47%)	N/A

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

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

Total install size change: ⬆️ 2.7 MB (4.47%)
Total download size change: ⬆️ 2.4 MB (9.82%)

Largest size changes

Item	Install Size Change
📝 splashBackground@3x.jpg	⬆️ 600.2 kB
📝 background@3x.jpg	⬆️ 368.6 kB
📝 boost-rewards@3x.png	⬆️ 188.4 kB
📝 background@2x.jpg	⬆️ 176.1 kB
📝 boost-rewards@2x.png	⬆️ 90.1 kB

View Treemap

🛸 Powered by Emerge Tools

satish-ravi · 2024-03-15T23:09:48Z

src/navigator/TabNavigator.tsx

@@ -47,6 +48,8 @@ export default function TabNavigator({ route }: Props) {
        options={{
          tabBarLabel: t('bottomTabsNavigator.wallet.tabName') as string,
          tabBarIcon: Wallet,
+          tabBarTestID: 'Tab/Wallet',
+          ...(tabHeader as NativeStackHeaderProps),


since this is the same across all screens, I think we can just set it once in the screenOptions prop of Tab.Navigator. Also is it possible to avoid the as casting?

Moved to screenOptions in d5cc11c.

All of our headers using the NativeStackNavigationOptions, but screenOptions in Tab.Navigator expects a type of NativeStackHeaderProps. I believe this is due with our nested navigators, but it works in all the cases I tested. I tried avoiding the type casting and using a ts-expect-error, but that wasn't working.

satish-ravi · 2024-03-16T00:24:08Z

src/dappsExplorer/DAppsExplorerScreenSearchFilter.tsx

+  }
+
+  const handleScroll = useAnimatedScrollHandler((event) => {
+    Animated.event([{ nativeEvent: { contentOffset: { y: scrollPositionValue } } }])


what's the purpose of this here? Seems like this returns an event handler function (that extracts contentOffset.y and sets it on scrollPositionValue) which should directly be passed to onScroll / other events so is this getting invoked actually? Seems like this also causes an error when scrolling

Moved to calling onScroll in e2846b7 and I no longer see the error.

satish-ravi · 2024-03-16T00:33:54Z

src/dappsExplorer/DAppsExplorerScreenSearchFilter.tsx

 import { StackParamList } from 'src/navigator/types'
 import { useDispatch, useSelector } from 'src/redux/hooks'
-import colors from 'src/styles/colors'
-import fontStyles from 'src/styles/fonts'
+import colors, { Colors } from 'src/styles/colors'


nit: could we just use the default export or the named export? I believe both point to the same object

Switched to using the named export in 74a55b5.

satish-ravi · 2024-03-16T00:35:07Z

src/home/WalletHome.tsx

+  }
+
+  const handleScroll = useAnimatedScrollHandler((event) => {
+    Animated.event([{ nativeEvent: { contentOffset: { y: scrollPositionValue } } }])


same question as above

Moved to calling onScroll in e2846b7 and I no longer see the error.

satish-ravi · 2024-03-18T19:20:50Z

src/tokens/Assets.tsx

@@ -266,8 +267,8 @@ AssetsScreen.navigationOptions = {
 const styles = StyleSheet.create({
  listHeaderContainer: {
    ...getShadowStyle(Shadow.SoftLight),
-    paddingHorizontal: Spacing.Thick24,
-    paddingTop: Spacing.Smallest8,
+    paddingHorizontal: Spacing.Regular16,


should we keep these at 24 for now, since the token balance items are spaced at 24?

Made it conditional base on the new tab view in 54e3825. The difference in header placement was a bit to noticeable in my opinion. Once we have a follow up PR to adjust padding we can remove the non tab navigator path.

satish-ravi · 2024-03-18T19:25:56Z

src/navigator/TabNavigator.test.tsx

+    jest.clearAllMocks()
+    jest
+      .mocked(getFeatureGate)
+      .mockImplementation((featureGate) => featureGate !== StatsigFeatureGates.USE_TAB_NAVIGATOR)


personally not a fan of this pattern as it sets every other gate to true, but I guess this is good enough for documenting what gate we really care about in these tests.

I am open to other suggestions. It looks like mockImplementation always expects a boolean value returned so it's a bit tricky.

satish-ravi · 2024-03-18T19:28:14Z

src/navigator/Headers.tsx

@@ -244,10 +262,12 @@ export function HeaderTitleWithTokenBalance({

 export function HeaderTitleWithSubtitle({
  title,
+  titleStyle,


wonder if we should just update this universally instead of a prop. Maybe worth checking with design?

Made it an optional prop to avoid impact on any of the existing screens. Currently this is used by the useScrollAwareHeader.tsx.

I was actually proposing changing every screen to the new style. Seems low risk to me, and avoids another style prop which has to be passed down 2 layers deep, and this would be something that is never cleaned up.

Changed all the title styles in beb06fe. A quick look through the app didn't reveal any issues with this change on both large and default fonts.

satish-ravi · 2024-03-18T19:44:11Z

src/components/AccountCircleButton.tsx

+
+export default function AccountCircleButton({ style, size, testID }: Props) {
+  const onPress = () => {
+    navigate(Screens.ProfileMenu)


should we add an analytics event for this?

Added in c4cf8fe!

satish-ravi · 2024-03-18T19:46:35Z

src/components/AccountCircleButton.tsx

+
+  return (
+    <View style={styles.container}>
+      <Touchable


is this something TopBarIconButton can support? if not, is there a purpose of keeping TopBarIconButton around?

MuckT · 2024-03-18T22:51:39Z

@satish-ravi in regards to keeping the TopBarIconButton.tsx it was used in too many other places to justify removing it in this PR. The main reason not to use is its lack of handing Android ripples. There are some notes in src/components/BackButton.tsx from @jeanregisser about what it took to get the ripple correct and it requires placement specific styling and seems like a general hassle. In my opinion we should create a better Icon wrapper and deprecate TopBarIconButton.tsx in a separate PR / ticket.

satish-ravi · 2024-03-19T00:06:31Z

src/navigator/Headers.tsx

@@ -244,10 +262,12 @@ export function HeaderTitleWithTokenBalance({

 export function HeaderTitleWithSubtitle({
  title,
+  titleStyle,


I was actually proposing changing every screen to the new style. Seems low risk to me, and avoids another style prop which has to be passed down 2 layers deep, and this would be something that is never cleaned up.

satish-ravi · 2024-03-19T00:16:47Z

src/home/WalletHome.tsx

+  }
+
+  const handleScroll = useAnimatedScrollHandler((event) => {
+    onScroll


Is this statement doing anything? onScroll I believe is a function, so this seems like just a line that references the function and doesn't invoke it.
Also, I think you should be able to use a single event handler instead of the 2 different handlers based on the gate. I believe both are just extracting event.contentOffset.y and setting it into a variable. onScroll sets it on the scrollPositionValue (of type Animated.Value) and handleScroll sets it on scrollPosition (of type SharedValue). We could just make all consumers use the SharedValue or just have a single handler that sets both variables.

I tried conditionally setting the scroll in fc03d3e. It looks like the other scroll is only being used by the drawer header so cleaning this up might be easier once that is removed. Perhaps a ticket to clean-up the drawer header and add todo's linked to that?

satish-ravi · 2024-03-19T00:26:24Z

@satish-ravi in regards to keeping the TopBarIconButton.tsx it was used in too many other places to justify removing it in this PR. The main reason not to use is its lack of handing Android ripples. There are some notes in src/components/BackButton.tsx from @jeanregisser about what it took to get the ripple correct and it requires placement specific styling and seems like a general hassle. In my opinion we should create a better Icon wrapper and deprecate TopBarIconButton.tsx in a separate PR / ticket.

that sounds good. Can we mark this deprecated now and link to a placeholder issue so no new stuff is added that uses this?

### Description Adds the top navigation for tab navigation screens. In the screen shots below, the header is the main point of focus as different wallets were used to take enabled and disabled screenshots. Header icons were adjusted to allow for ripples on tap on Android and a few changes had to be made to keep alignment with the existing side `DrawerTopBar.tsx`. #### Screenshots | Android Disabled | Android Enabled | iOS Disabled | iOS Enabled | | ----- | ----- | ----- | ----- | | ![](https://github.com/valora-inc/wallet/assets/26950305/51005dd6-ceaf-43a3-8e98-a46902cdae63 "Android Tab Nav Disabled") | ![](https://github.com/valora-inc/wallet/assets/26950305/db0630b1-c0bb-44a4-8720-427f2f5599bf "Android Tab Nav Enabled") | ![](https://github.com/valora-inc/wallet/assets/26950305/cddf91e4-9eab-4d25-8e8d-2629d53ebc13 "iOS Tab Nav Disabled") | ![](https://github.com/valora-inc/wallet/assets/26950305/d2ad89d8-9a9f-4bcd-8621-a298f1916f7c "iOS Tab Nav Enabled") | ### Test plan - [x] Tested locally on iOS - [x] Tested locally on Android - [x] Unit tests added ### Related issues - Fixes #ACT-1108 ### Backwards compatibility Yes ### Network scalability N/A

MuckT added 5 commits March 12, 2024 18:48

chore: add title for discover screen

fafb4ac

chore: add account circle button

c2bd286

chore: add title style to header

11bd86b

feat: use new header on wallet assets

d2f0fc9

Merge branch 'main' into tomm/act-1108

932b4ec

MuckT and others added 8 commits March 13, 2024 17:36

feat: create shared header for tab navigator screens

a544525

chore: use shared tab header

dddd0c4

fix: wrap with animated view for header scroll on android

fdba8da

feat: handle scrollable header in discover and home tabs

2ffe340

test: adjust for multiple matching text elements

0cdac6b

Merge branch 'main' into tomm/act-1108

95a5c43

fix: remove unnecessary wrapper causing test failures

2aeef88

test(tab-nav): check labels and navigation

26ffb10

MuckT marked this pull request as ready for review March 15, 2024 20:09

MuckT added 3 commits March 15, 2024 14:15

feat: ripple for header items on android

926aa3a

fix: right header items for wallet home

585a26f

fix: move test id to touchable

9551481

MuckT commented Mar 15, 2024

View reviewed changes

MuckT added 2 commits March 15, 2024 15:32

fix: maintain existing onScroll in new handleScroll

8111b63

docs: nested animated view fix for android

2597590

satish-ravi reviewed Mar 18, 2024

View reviewed changes

MuckT added 3 commits March 18, 2024 14:44

fix: call on scroll

e2846b7

style: use tab header in shared screen options

d5cc11c

style: prefer named export

74a55b5

MuckT added 4 commits March 18, 2024 15:18

chore: add analytics for account circle tapped

c4cf8fe

fix: rename refresh control colors prop to proper case

51af4d9

chore: conditionally adjust padding for new tab view

54e3825

docs: add missing description for account circle tapped

af9d1c8

satish-ravi reviewed Mar 19, 2024

View reviewed changes

MuckT and others added 7 commits March 18, 2024 22:35

fix: call function on scroll in handle scroll

ce2177b

fix: conditionally set scroll

fc03d3e

style: simplify titleStyle type

f588a88

Merge branch 'main' into tomm/act-1108

f70e4ca

fix: conditionally call scroll handler on tab nav availability

3a4ac76

docs: link cleanup todos to cleanup issue in linear

2133d98

chore: change header title styles

beb06fe

satish-ravi approved these changes Mar 19, 2024

View reviewed changes

chore: deprecate TopBarIconButton

f155a47

MuckT enabled auto-merge March 20, 2024 00:09

MuckT disabled auto-merge March 20, 2024 00:14

MuckT enabled auto-merge March 20, 2024 00:21

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

Merged via the queue into main with commit f529920 Mar 20, 2024
16 checks passed

MuckT deleted the tomm/act-1108 branch March 20, 2024 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add tab bar header navigation #5082

feat: add tab bar header navigation #5082

MuckT commented Mar 13, 2024 •

edited

codecov bot commented Mar 13, 2024 •

edited

MuckT Mar 15, 2024

satish-ravi Mar 18, 2024

MuckT Mar 15, 2024

MuckT Mar 15, 2024

MuckT Mar 15, 2024

MuckT Mar 15, 2024 •

edited

emerge-tools bot commented Mar 18, 2024 •

edited

satish-ravi Mar 15, 2024

MuckT Mar 18, 2024 •

edited

MuckT Mar 18, 2024

satish-ravi Mar 16, 2024

MuckT Mar 18, 2024

satish-ravi Mar 16, 2024

MuckT Mar 18, 2024

satish-ravi Mar 16, 2024

MuckT Mar 18, 2024

satish-ravi Mar 18, 2024

MuckT Mar 18, 2024

satish-ravi Mar 18, 2024

MuckT Mar 18, 2024 •

edited

satish-ravi Mar 18, 2024

MuckT Mar 18, 2024

satish-ravi Mar 19, 2024

MuckT Mar 19, 2024

satish-ravi Mar 18, 2024

MuckT Mar 18, 2024

satish-ravi Mar 18, 2024

MuckT commented Mar 18, 2024

satish-ravi Mar 19, 2024

satish-ravi Mar 19, 2024

MuckT Mar 19, 2024

satish-ravi commented Mar 19, 2024

feat: add tab bar header navigation #5082

feat: add tab bar header navigation #5082

Conversation

MuckT commented Mar 13, 2024 • edited

Description

Screenshots

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Mar 13, 2024 • edited

Codecov Report

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

MuckT Mar 15, 2024 • edited

Choose a reason for hiding this comment

emerge-tools bot commented Mar 18, 2024 • edited

1 build increased size

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

Choose a reason for hiding this comment

MuckT Mar 18, 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

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

Choose a reason for hiding this comment

MuckT Mar 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuckT commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satish-ravi commented Mar 19, 2024

MuckT commented Mar 13, 2024 •

edited

codecov bot commented Mar 13, 2024 •

edited

MuckT Mar 15, 2024 •

edited

emerge-tools bot commented Mar 18, 2024 •

edited

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

MuckT Mar 18, 2024 •

edited

MuckT Mar 18, 2024 •

edited