feat: refactor navigation infra #10977

MounirDhahri · 2024-10-18T16:23:23Z

Description

This PR started as a Future Friday effort to spike on a potential hybrid approach of adopting React-Navigation in the App.

After a few commits, I realized this was worth pursuing and kept working on it to get us to a mergeable presentable state.

What does this PR bring?

iOS and Android navigation is now aligned. using react-navigation
Noticeable performance improvements: install the app and feel it by yourself!
Many many fixes to unaligned screen headers
- This was achieved mostly thanks to using default navigation headers from react-navigation (and styling them like we do)

What does this PR not do

No changes to the deep linking logic
No changes to route names
No changes to the bottom tabs
- I tried to, and it worked but I figured out it's better to keep that separate in order not to bring any visual changes.

What does this PR allow us to

Align our headers across the app with minimal effort
Remove the old ModalStack NavStack and a bunch of native features about navigation.
Once we upgrade to React-navigation 7, we will be able to take advantage of exciting new features like preload link
Introduce a better pattern for adding new screens and defining their layout. Just like we do in Force
Less old native code means less tech debt means less obstacles when it comes to trying out some new exciting features.

Notes for the reviewers

This PR can be reviewed commit by commit. If you want a journey in my mind and a journey to how I introduced things. I encourage you to to review it in order of commits. I added a guided tour that you can start by clicking here 👉👉🔗 First commit: Add Feature Flag
Are there any resources I can watch in order to understand better what we currently have and where we want to be? Join the practice mobile to learn more about it! I am presenting today

What comes next

Introduce Unauthorized flow logic
Align on a new format for defining modules and screens. We need a thread around that to ensure everyone is aligned.
Simplification to our deep linking logic by relying more on react-navigation
Remove old code
Upgrade react-navigation 7

How can I test things out

Enable "Enable new navigation infra (requires app restart feature flag)"
Install ios-8.55.0-2024.10.29.13 or android-8.55.0-1674645910 betas

PR Checklist

I have tested my changes on iOS and Android.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

I would like at least one of the reviewers to run this PR on the simulator or device.

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

feat: refactor navigation infra - mounir

Need help with something? Have a look at our docs, or get in touch with us.

src/app/Navigation/AuthenticatedRoutes/HomeTab.tsx

araujobarret

Codewise looks good 👍
To clarify this PR is only about reorg of nav stuff, right?

araujobarret · 2024-10-21T12:14:25Z

src/app/Navigation/AuthenticatedRoutes/HomeTab.tsx

+export const HomeTab = () => {
+  return (
+    <TabStackNavigator.Navigator initialRouteName="Home" screenOptions={{ headerShown: false }}>
+      {SharedRoutes()}


can't we just <SharedRoutes /> instead? I noticed the typings there need to be React.FC too

The reason why this wasn't possible, was because StackNavigator.Navigator expects its children to be of type Screen, Group or Fragment.

(follow-up PR): We can make our component with this type, right? Usually, people use these extra guard-rails to avoid using wrong children, but it's just a type check in their code.

Apologies, I didn't get what you mean here. Do you mind if you explain it to me in code so I get better what would you like this API to look like.

src/app/Navigation/AuthenticatedRoutes/SharedRoutes.tsx

src/app/Navigation/AuthenticatedRoutes/Tabs.tsx

src/app/Navigation/Navigation.tsx

gkartalis · 2024-10-23T13:53:17Z

8.55.0 - 2024.10.23.12
android-8.55.0-1674645897

MounirDhahri · 2024-10-28T15:00:01Z

src/app/Scenes/City/Components/Event/index.tsx

-                <Box mb={2}>
-                  <OpaqueImageView imageURL={url} height={145} />
+                <Box mb={2} justifyContent="center" overflow="hidden">
+                  <Image src={url} height={145} />


Finally fixed this issue which was there since years

This reverts commit 4cb2960.

MounirDhahri · 2024-11-06T13:18:10Z

Hey @damassi - sorry for the late reply, I was sick and got back today. The PR is already reviewable actually and I can address any comments.
My goal is to get this merged by end of week and schedule a QA session for beginning of next week.

MounirDhahri · 2024-11-07T08:48:38Z

Requests are welcome - I won't push more to this and we can have the unauthorized flow in a separate PR.

damassi

This is a big PR and I don't have many comments other than its nice to get this all into one system, and its a very good improvement.

Couple things:

Once this is merged, we should prioritize getting in the other unauthenticated routes so that we're not in a split situation
We should also prioritize, sooner rather than later, removing the FF split between old and new, so that we're on one system. Once that's done i think we can do a cleanup/polish pass and this will be 👌

@brainbicycle, @gkartalis - do y'all have any thoughts?

araujobarret

Looking good, thanks for the hard work here 💪
Left a small comment for a follow-up PR

src/app/Navigation/AuthenticatedRoutes/StackNavigator.tsx

araujobarret · 2024-11-08T08:20:37Z

src/app/Navigation/AuthenticatedRoutes/HomeTab.tsx

+export const HomeTab = () => {
+  return (
+    <TabStackNavigator.Navigator initialRouteName="Home" screenOptions={{ headerShown: false }}>
+      {SharedRoutes()}


(follow-up PR): We can make our component with this type, right? Usually, people use these extra guard-rails to avoid using wrong children, but it's just a type check in their code.

olerichter00

Thanks for the great work! 🌟

I didn't take the time to check all the code changes in detail, but overall, it looks pretty good 👍

gkartalis

Awesome work! I did leave some questions but none are blocking.

Another generic question that I have is around the headers across the app, lets say that design/product doesn't agree with the header changes is there a way to change the headers across the app with one custom component easily now with the react navigation?

gkartalis · 2024-11-08T09:25:46Z

src/app/Navigation/AuthenticatedRoutes/NativeScreens.tsx

+if (Platform.OS === "ios") {
+  modules.map(({ moduleName, module }) => {
+    register(moduleName, module)
+  })
+}


gkartalis · 2024-11-08T09:27:33Z

src/app/Navigation/AuthenticatedRoutes/StackNavigator.tsx

+  readonly hidesBottomTabs?: boolean
+}
+
+export const ScreenWrapper: React.FC<ScreenWrapperProps> = ({


question(non-blocking): how is the screen wrapper different than our <Screen /> component from palette? Would it make sense to use the Screen here instead of this custom one?

I wanted actually to use Screen from palette-mobile initially here but that meant that I would have to go through all screens and fix their padding and in some cases, remove Screen from the screens that are using it. And that felt to me something too early to do and it would require a separate PR for it. So I just copied the existing sreen wrapper that we currently have in AppRegistry and simplified it a bit. A future step would be to have no wrapper logic here but it's probably too early for it now

gkartalis · 2024-11-08T09:31:07Z

src/app/Scenes/Article/Components/ArticleWebViewScreen.tsx

-        {/*
-            NOTE: we don't need safeAreaEdges but passing undefined or empty array didn't work,
-            so we're passing "left" that doesn't actually add anything to the webview to avoid
-            having double paddings from Screen and ArtsyWebView
-        */}
-        <ArtsyWebView url={data.href} safeAreaEdges={["left"]} />


question(non-blocking): why did we get rid of safeAreaEdges here?

Good question, the reason behind that here was that after my changes, this wasn't needed anymore and there were no more paddings. But good note on this because It means I need to bring them back in case the feature flag is disabled

gkartalis · 2024-11-08T09:34:13Z

src/app/Scenes/ArtworkMedium/ArtworkMedium.tsx

  const space = useSpace()
+  const enableNewNavigation = useFeatureFlag("AREnableNewNavigation")
+
+  useAndroidGoBack()


I'd be curious to understand if we do need this still after this PR stabilizes though and we do remove the feature flag

(for context: this was added in some places of the app because without it it was navigating us to the MyCollection screen when pressing the android back button instead of going back on the navigation stack probably because of our hybrid nav setup)

good note. Let me test it out actually

Nice! it's working without it now. I will remove it

Screen.Recording.2024-11-08.at.11.23.49.mov

gkartalis · 2024-11-08T09:35:10Z

src/app/Scenes/BottomTabs/BottomTabs.tsx

@@ -18,6 +18,7 @@ export const BottomTabs: React.FC<BottomTabBarProps> = (props) => {
  const focusedRoute = findFocusedRoute(props.state)
  const params = focusedRoute?.params as any
  const module = modules[params?.moduleName as AppModule]
+  // const unreadConversationsCount = 3


👁️ should we get rid of this?

oh yes definitely

gkartalis · 2024-11-08T09:36:14Z

src/app/Scenes/AuctionResult/AuctionResult.tsx

+  useLayoutEffect(() => {
+    navigation.setOptions({
+      headerTitle: `${auctionResult.title}, ${auctionResult.dateText}`,
+    })
+  }, [navigation])


how does the headerTitle show up if the title and date text is too long to be displayed? If we do use our <Text> component in order to render this text we are okay since this is handled

by default, react-navigation shows ellipsis dots if it's too long. We can in the future use our custom ellipsis mode there but since we are not doing that currently I didn't update it

gkartalis · 2024-11-08T09:40:25Z

src/app/Scenes/MyAccount/MyAccountEditEmail.tsx

+  useEffect(() => {
+    navigation.setOptions({
+      headerRight: () => {
+        return (
+          <Touchable onPress={handleSave} disabled={!isEmailValid}>
+            <Text variant="xs" color={isEmailValid ? "black100" : "black60"}>
+              Save
+            </Text>
+          </Touchable>
+        )
+      },
+    })
+  }, [navigation, email])


since this is a static component (if I understand this correctly) why do we have it set in a use effect? 🤔

Here I am doing that to take advantage of react-navigation defined headers. (title and headerLeft and rightHeader). We could define this in component withing the render tree but then we would need maintain the header in this component separately.
News is that react-navigation might be migrating to a different API where we would be defining that header withing the render method. 🤞

gkartalis · 2024-11-08T09:43:48Z

src/app/system/devTools/DevMenu/DevMenu.tsx

-          Email: <Text variant="xs">{userEmail}</Text>
-        </Text>
+  useEffect(() => {
+    queueMicrotask(() => {


why do we use queue microtask + hide the setOptions here with a feature flag versus the other places where we do set the options for the header right item without queueMicrotask + featureflag?

Sharp, this is not neeeded. I will remove it.

For context, initially, I was showing modal screens just with a different presentation way and hide their bottom tabs. That led to too much stuff happening on the UI and this useEffect was breaking the UI so I queued it. However later in the PR, I realized that it makes more sense to actually have the modals be modals and not deal with any issues like this

MounirDhahri · 2024-11-08T14:48:13Z

Did a first round of the existing infra QA to make sure I didn't break anything. I will merge this PR in an hour then send an invitation for QA for next week. Thanks everyone for reviewing.

MounirDhahri · 2024-11-08T16:04:00Z

Only thing left to do now is hooking up the unauthorized flow. Everything else should be working fine

Merging this now.

🚀 🚀 🚀 🚀 🚀

damassi · 2024-11-08T16:35:43Z

Nice work @MounirDhahri 💥

MounirDhahri self-assigned this Oct 18, 2024

damassi reviewed Oct 18, 2024

View reviewed changes

src/app/Navigation/AuthenticatedRoutes/HomeTab.tsx Outdated Show resolved Hide resolved

araujobarret reviewed Oct 21, 2024

View reviewed changes

MounirDhahri marked this pull request as draft October 22, 2024 15:46

gkartalis force-pushed the feat/update-navigation-infra branch from d61825c to 9e156cb Compare October 23, 2024 12:29

MounirDhahri force-pushed the feat/update-navigation-infra branch 8 times, most recently from 5d9716a to da45c6a Compare October 28, 2024 12:04

MounirDhahri commented Oct 28, 2024

View reviewed changes

MounirDhahri added 15 commits October 29, 2024 10:22

chore: add feature flag

51de304

feat: prepare main stack

6f5ab1d

feat: add tabs stack

c9919ce

feat: hook up the different screens

356284c

feat: update other navigate methods to support new navigation infra

fbd6049

chore: add hides back button to tabs

7a30028

feat: update tabs styling

2bd3d73

chore: remove console.log

0b2c983

chore: update live sale logic

55a927d

chore: remove unused prop

a44176f

feat: remove tab screens from shared routes

58220e8

chore: remove comment

b7166d4

chore: update screen wrapper props

4e0e226

feat: add tab screens to the individual tabs

d03ce95

chore: add todo comment

147123c

MounirDhahri added 8 commits November 1, 2024 10:15

Revert "fix: align simple page with header with current designs"

fc25b84

This reverts commit 4cb2960.

fix: align (again) simple page with header with current designs

09b8c43

fix: annoying spinner on add artwork to my collection

7c3360d

feat: introduce new modal stack

1a46ab4

fix: auction bidding modal

09d5495

fix: make sure privacy and terms are displayed in modals

0543d87

fix: typescript

64cc549

Merge branch 'main' into feat/update-navigation-infra

a101cbc

MounirDhahri added 2 commits November 7, 2024 09:42

fix: broken test

bbb9ae7

Merge branch 'main' into feat/update-navigation-infra

26601ad

Merge branch 'main' into feat/update-navigation-infra

961d64f

damassi previously approved these changes Nov 7, 2024

View reviewed changes

araujobarret previously approved these changes Nov 8, 2024

View reviewed changes

olerichter00 previously approved these changes Nov 8, 2024

View reviewed changes

gkartalis previously approved these changes Nov 8, 2024

View reviewed changes

fix: address review comments

7cdd53e

MounirDhahri dismissed stale reviews from gkartalis, olerichter00, araujobarret, and damassi via 7cdd53e November 8, 2024 10:41

MounirDhahri added 2 commits November 8, 2024 12:19

fix: skip not needed broken test

834e38b

fix: bring back old presentation of screens in app registry

1946117

MounirDhahri merged commit 5fcfa7d into main Nov 8, 2024
7 checks passed

MounirDhahri deleted the feat/update-navigation-infra branch November 8, 2024 16:04

MounirDhahri mentioned this pull request Nov 22, 2024

feat: simplify bottom tabs logic #11175

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor navigation infra #10977

feat: refactor navigation infra #10977

MounirDhahri commented Oct 18, 2024 •

edited

Loading

araujobarret left a comment

araujobarret Oct 21, 2024

MounirDhahri Oct 29, 2024

araujobarret Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis commented Oct 23, 2024

MounirDhahri Oct 28, 2024 •

edited

Loading

MounirDhahri commented Nov 6, 2024

MounirDhahri commented Nov 7, 2024

damassi left a comment

araujobarret left a comment

araujobarret Nov 8, 2024

olerichter00 left a comment

gkartalis left a comment

gkartalis Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

gkartalis Nov 8, 2024

MounirDhahri Nov 8, 2024

MounirDhahri Nov 8, 2024

MounirDhahri commented Nov 8, 2024

MounirDhahri commented Nov 8, 2024

damassi commented Nov 8, 2024

feat: refactor navigation infra #10977

feat: refactor navigation infra #10977

Conversation

MounirDhahri commented Oct 18, 2024 • edited Loading

Description

Notes for the reviewers

What comes next

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

araujobarret 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

gkartalis commented Oct 23, 2024

MounirDhahri Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

MounirDhahri commented Nov 6, 2024

MounirDhahri commented Nov 7, 2024

damassi left a comment

Choose a reason for hiding this comment

araujobarret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olerichter00 left a comment

Choose a reason for hiding this comment

gkartalis 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

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

MounirDhahri commented Nov 8, 2024

MounirDhahri commented Nov 8, 2024

damassi commented Nov 8, 2024

MounirDhahri commented Oct 18, 2024 •

edited

Loading

MounirDhahri Oct 28, 2024 •

edited

Loading