feat: Android nav #4362

ds300 · 2021-01-26T15:26:41Z

Description

This PR introduces navigation stacks for Android (and eventually iOS) using react-navigation + react-native-screens. This means we are using platform native navigation primitives under the hood, for speed 🚗

This PR also puts Artsy's iOS-only native modules (e.g. ARTemporaryAPIModule) behind a LegacyNativeModules object to emphasise that they are old, and many of them are ripe for removal as we get Android up and running.

nav-stacks.mp4

admin-menu.mp4

Follow up work

add tests for nav infra (deferring this to keep velocity high and avoid testing things which are liable to completely change when we adopt react-navigation on iOS too) https://artsyproduct.atlassian.net/browse/CX-982
add scroll-to-top support on bottom tab press https://artsyproduct.atlassian.net/browse/CX-983

PR Checklist (tick all before merging)

I have included screenshots or videos to illustrate my changes, or I have not changed anything that impacts the UI.
I have added tests for my changes, or my changes don't require testing, or I have included a link to a separate Jira ticket covering the tests.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added an app state migration, or my changes do not require one. (What are migrations?)
I have added a CHANGELOG.yml entry or my changes do not require one.

ds300 · 2021-01-29T14:09:24Z

package.json

@@ -84,7 +84,9 @@
    "@react-native-community/masked-view": "^0.1.10",
    "@react-native-community/netinfo": "4.6.1",
    "@react-native-community/picker": "1.3.0",
+    "@react-native-community/viewpager": "^4.2.2",


this was a transitive dependency of react-native-scrollable-tab-view that we had to make explicit, otherwise the android autolinking doesn't work.

ds300 · 2021-01-29T14:18:33Z

src/lib/Components/ArtworkGrids/ArtworkGridItem.tsx

-          <OpaqueImageView
-            aspectRatio={artwork.image?.aspectRatio ?? 1}
-            imageURL={artwork.image?.url}
-            style={styles.artworkImage}
-          >
+          <View>
+            <OpaqueImageView aspectRatio={artwork.image?.aspectRatio ?? 1} imageURL={artwork.image?.url} />
            {Boolean(!hideUrgencyTags && urgencyTag && artwork?.sale?.isAuction && !artwork?.sale?.isClosed) && (
-              <Flex backgroundColor="white" px="5px" py="3px" borderRadius={2} alignSelf="flex-start">
+              <Flex
+                position="absolute"
+                bottom="5px"
+                left="5px"
+                backgroundColor="white"
+                px="5px"
+                py="3px"
+                borderRadius={2}
+                alignSelf="flex-start"
+              >
                <Sans size="2" color="black100" numberOfLines={1}>
                  {urgencyTag}
                </Sans>
              </Flex>
            )}
-          </OpaqueImageView>
+          </View>


Lots of screens were causing the app to crash because a few commonly-used components were putting children inside an OpaqueImageView. On android OpaqueImageView currently uses the normal react-native Image component under the hood, which doesn't support children like our iOS native image component, so we decided to just refactor the few places where this was being done.

ds300 · 2021-01-29T16:13:24Z

src/lib/Scenes/BottomTabs/BottomTabsNavigator.tsx

+          <View
+            key={index}
+            pointerEvents={index === activeIndex ? undefined : "none"}
+            style={{ position: "absolute", width: "100%", height: "100%" }}
+          >
+            <Animated.View style={{ opacity: opacities[index], flex: 1, backgroundColor: "white" }}>
+              {!!(hasLoaded[index] || index === activeIndex) && v}
+            </Animated.View>
+          </View>


We refactored this component because there seemed to be an Animated bug on android during fade transitions. When animating the next view from opacity 0 to 1 the first frame would have opacity 1 already for some reason. e.g. if there were 10 frames in the animation, the opacity at each frame should be like [0, 0.1, 0.2, ..., 1] but we were seeing [1, 0.1, 0.2, ..., 1].

Most confusing.

Anyway we made the problem disappear by moving things around.

ds300 · 2021-01-29T16:17:53Z

src/lib/navigation/BackButton.tsx

+      <TouchableOpacity onPress={onPress}>
+        <ChevronIcon direction="left" />
+      </TouchableOpacity>


💡 At first this was using our custom Touchable but when debugging on android the onPress event was not being fired, no clue why. Most bizarre.

Hm, we could take a look at a KS.

patches/@react-navigation+core+5.14.0.patch

ds300 · 2021-02-01T12:10:36Z

android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java

+    public static void didLaunch(SharedPreferences prefs) {
+        launchCount = prefs.getInt(LAUNCH_COUNT, 0) + 1;
+        prefs.edit().putInt(LAUNCH_COUNT, launchCount).commit();
+        System.out.println("LaunchCount:" + launchCount);
+    }


We need the launchCount at dev time for maintaining navigation state across reloads.

We want the behaviour to be:

If you relaunch the app, the navigation state should reset

If you reload the js bundle, the navigation state should kept

We couldn't find a built-in way to tell the difference between relaunch and reload, so we added this launchCount property that is incremented only when the app launches. We already had it available on iOS.

Smort! Nice one.

ds300 · 2021-02-01T12:15:30Z

src/lib/Components/FilterModal/FilterModal.tsx

-    <NavigationContainer>
+    <NavigationContainer independent>


We now have a NavigationContainer wrapping the main part of the app (after onboarding). So any places where we use react-navigation in modals, e.g. here, we need to set this independent prop to true, which just silences a console.error that is intended for people who are using react-navigation in the normal way.

ds300 · 2021-02-01T12:39:37Z

src/lib/utils/AdminMenuWrapper.tsx

+      onStartShouldSetResponderCapture={() => {
+        const now = Date.now()
+        const state = gestureState.current
+
+        if (now - state.lastTapTimestamp < 500) {
+          state.numTaps += 1
+        } else {
+          state.numTaps = 1
+        }
+
+        state.lastTapTimestamp = now
+
+        if (state.numTaps >= 5) {
+          state.numTaps = 0
+          setIsShowingAdminMenu(true)
+        }
+        return false
+      }}


I added a placeholder admin menu already, to allow us to clear AsyncStorage easily during development. For now I just made it trigger on five taps anywhere, with less than 0.5 seconds between each tap. We can iterate on this.

It might be good to have like a three finger hold or something, but yea, that's for the admin jira ticket 👍.

only problem with a three finger hold is, how to do that on a simulator?

Hm.. true.. ok we will figure it out in the other ticket. I only don't like the spam tapping because it's easy to keep tapping and then tap something on the admin menu 😬.

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.t>

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

ds300 · 2021-02-01T14:19:22Z

hmm QAing this and it seems to have affected touch handling on the FilterModal on iOS, I am investigating, don't merge yet.

ds300 · 2021-02-01T18:17:48Z

OK @pvinis this is good to go

MounirDhahri · 2021-02-01T19:21:08Z

src/lib/navigation/BackButton.tsx

+    return null
+  }
+  return (
+    <Animated.View


Do you need this to be an Animated.View

android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java

MounirDhahri · 2021-02-01T20:26:04Z

typings/emission.d.ts

@@ -1,67 +0,0 @@
-import type { ViewDescriptor } from "lib/navigation/navigate"


MounirDhahri · 2021-02-01T20:30:55Z

src/lib/Components/FilterModal/FilterModal.tsx

@@ -180,6 +180,8 @@ export const FilterModalNavigator: React.FC<FilterModalProps> = (props) => {
      <FancyModal visible={props.isFilterArtworksModalVisible} onBackgroundPressed={handleClosingModal} maxHeight={550}>
        <View style={{ flex: 1 }}>
          <Stack.Navigator
+            // force it to not use react-native-screens, which is broken inside a react-native Modal for some reason
+            detachInactiveScreens={false}


this is an interesting prop!

MounirDhahri · 2021-02-01T20:31:27Z

🔥🔥🔥

android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java

pvinis

Very very cool work from both of you!

pvinis · 2021-02-02T08:17:40Z

android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java

+    public static void didLaunch(SharedPreferences prefs) {
+        launchCount = prefs.getInt(LAUNCH_COUNT, 0) + 1;
+        prefs.edit().putInt(LAUNCH_COUNT, launchCount).commit();
+        System.out.println("LaunchCount:" + launchCount);
+    }


Smort! Nice one.

pvinis · 2021-02-02T08:22:24Z

src/lib/NativeModules/ARScreenPresenterModule.tsx

+import { NativeModules } from "react-native"
+
+/**
+ * Here we maintain references to all the navigators in the main app navigation hierarchy, which are:


pvinis · 2021-02-02T08:31:07Z

src/lib/navigation/BackButton.tsx

+      <TouchableOpacity onPress={onPress}>
+        <ChevronIcon direction="left" />
+      </TouchableOpacity>


Hm, we could take a look at a KS.

pvinis · 2021-02-02T08:36:35Z

src/lib/navigation/ModalStack.tsx

+/**
+ * ModalStack is the root navigation stack in the app. The root screen in this stack is
+ * the main app (with bottom tabs, etc), and then whenever we present a modal it gets
+ * pushed on the top of this stack. We use react-native-screens to get native modal presetation
+ * transitions etc.
+ */


These comments are very nice and helpful! I'm sure they will help make everything clear to every dev that comes in here.

pvinis · 2021-02-02T08:47:26Z

src/lib/utils/AdminMenuWrapper.tsx

+export const AdminMenuWrapper: React.FC = ({ children }) => {
+  const [isShowingAdminMenu, setIsShowingAdminMenu] = useState(false)
+  const gestureState = useRef({ lastTapTimestamp: 0, numTaps: 0 })
+  // TODO: if public release, just return <>{children}</>


I would say we keep it in release too, still useful as it is now.

pvinis · 2021-02-02T08:48:25Z

src/lib/utils/AdminMenuWrapper.tsx

+      onStartShouldSetResponderCapture={() => {
+        const now = Date.now()
+        const state = gestureState.current
+
+        if (now - state.lastTapTimestamp < 500) {
+          state.numTaps += 1
+        } else {
+          state.numTaps = 1
+        }
+
+        state.lastTapTimestamp = now
+
+        if (state.numTaps >= 5) {
+          state.numTaps = 0
+          setIsShowingAdminMenu(true)
+        }
+        return false
+      }}


It might be good to have like a three finger hold or something, but yea, that's for the admin jira ticket 👍.

# Conflicts: # Podfile.lock # package.json

artsy-peril bot added the Jira Synced label Jan 26, 2021

ds300 changed the title ~~Android nav~~ Android 'quit early if possible' ci Jan 26, 2021

ds300 changed the title ~~Android 'quit early if possible' ci~~ [wip] android nav Jan 26, 2021

ds300 force-pushed the android-nav branch from 6925abb to c6b5329 Compare January 28, 2021 13:05

ds300 commented Jan 29, 2021

View reviewed changes

damassi reviewed Jan 30, 2021

View reviewed changes

patches/@react-navigation+core+5.14.0.patch Show resolved Hide resolved

ds300 commented Feb 1, 2021

View reviewed changes

ds300 changed the title ~~[wip] android nav~~ Android nav Feb 1, 2021

ds300 changed the title ~~Android nav~~ feat: Android nav Feb 1, 2021

ds300 assigned pvinis Feb 1, 2021

ds300 requested a review from MounirDhahri February 1, 2021 13:19

ds300 force-pushed the android-nav branch from dacf0bd to 879cd65 Compare February 1, 2021 13:27

ds300 and others added 13 commits February 1, 2021 13:34

make BottomTabNavigator work

c50268e

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

copy nav stack from spike, make root views work

8cb27b5

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

make stack navigation work

1df7a08

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

add and integrate modal stack

9e5edf3

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

remove instances of OpaqueImageView with children

f9947cc

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

add @react-native-community/viewpager as explicit dep

edd105d

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

add dev nav state persistence, handle modal nav properly

b1cf9f6

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

add launch count to android for managing dev reload state

dc5b9c4

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

ignore all require cycle warnings

125dede

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.t>

clean up nav stack dev reload state

13119de

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

fix bottom tabs dev reload state saving

b589844

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

fix bottom tabs navigator tab switch transition on android

b41d7c3

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

remove stray console.log

3f59419

Co-authored-by: Mounir Dhahri <mounir.dhahri@ensi-uma.tn>

ds300 added 12 commits February 1, 2021 13:35

cleanup logbox warning ignores

5e2d006

add comments for ARScreenPresenterModule.tsx

adbe802

reinstate useNativeDriver: true on FadeBetween

8a4ade6

simplify and improve nav state persistence

cd5aed5

remove redundant sanity check

6e1aa1c

refactor ArtsyNativeModules -> LegacyNativeModules

1492668

add ArtsyNativeModule file

9411838

add hacks.md entry for react-navigation patch

d291978

fix stackAnimation on NavigatorIOS shim

ddb95f3

update changelog

59dfb33

lint fix

636c597

add comments for LegacyNativeModules

cb8992b

ds300 force-pushed the android-nav branch from 879cd65 to cb8992b Compare February 1, 2021 13:35

ds300 added 2 commits February 1, 2021 13:51

fix import in ErrorReporting

cf24e15

update podfile

f9c013e

ds300 added 2 commits February 1, 2021 18:15

avoid react-native-screens in RN Modal

484f543

Merge branch 'master' into android-nav

62c093a

MounirDhahri approved these changes Feb 1, 2021

View reviewed changes

pvinis reviewed Feb 2, 2021

View reviewed changes

android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java Outdated Show resolved Hide resolved

Update android/app/src/main/java/net/artsy/app/ArtsyNativeModule.java

cf9cba1

pvinis approved these changes Feb 2, 2021

View reviewed changes

pvinis added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Feb 2, 2021

pvinis added 2 commits February 2, 2021 12:56

Merge branch 'master' into android-nav

5282155

# Conflicts: # Podfile.lock # package.json

Merge branch 'android-nav' of github.com:artsy/eigen into android-nav

401adf8

artsy-peril bot merged commit 7710872 into master Feb 2, 2021

artsy-peril bot deleted the android-nav branch February 2, 2021 09:41

pvinis mentioned this pull request Jul 1, 2022

chore(deps): remove unused dependencies #7016

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Android nav #4362

feat: Android nav #4362

ds300 commented Jan 26, 2021 •

edited

Loading

ds300 Jan 29, 2021

ds300 Jan 29, 2021

ds300 Jan 29, 2021

ds300 Jan 29, 2021

pvinis Feb 2, 2021

ds300 Feb 1, 2021

pvinis Feb 2, 2021

ds300 Feb 1, 2021

ds300 Feb 1, 2021

pvinis Feb 2, 2021

ds300 Feb 2, 2021

pvinis Feb 2, 2021

ds300 commented Feb 1, 2021

ds300 commented Feb 1, 2021

MounirDhahri Feb 1, 2021

MounirDhahri Feb 1, 2021

MounirDhahri Feb 1, 2021

MounirDhahri commented Feb 1, 2021

pvinis left a comment

pvinis Feb 2, 2021

pvinis Feb 2, 2021

pvinis Feb 2, 2021

pvinis Feb 2, 2021

pvinis Feb 2, 2021

pvinis Feb 2, 2021

		@@ -1,67 +0,0 @@
		import type { ViewDescriptor } from "lib/navigation/navigate"

feat: Android nav #4362

feat: Android nav #4362

Conversation

ds300 commented Jan 26, 2021 • edited Loading

Description

Follow up work

PR Checklist (tick all before merging)

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

Choose a reason for hiding this comment

ds300 commented Feb 1, 2021

ds300 commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounirDhahri commented Feb 1, 2021

pvinis left a 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

ds300 commented Jan 26, 2021 •

edited

Loading