Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Header Height jumps with nested navigators #619

Closed
dotconnor opened this issue Aug 30, 2020 · 31 comments · Fixed by #1029
Closed

Header Height jumps with nested navigators #619

dotconnor opened this issue Aug 30, 2020 · 31 comments · Fixed by #1029

Comments

@dotconnor
Copy link

When you have a nested navigators like this: NativeStack -> BottomTabs -> NativeStack, it creates issues when rendering the header from the innermost NativeStack. Only appears to happen when there some other navigator anywhere between two NativeStack ones. For example Stack -> BottomTabs -> NativeStack works correctly and NativeStack -> NativeStack work correctly.

This seems to be the same bug as #540.

https://github.com/dotconnor/ScreensHeader

Expected

Kapture 2020-08-30 at 05 47 11

Actual

Kapture 2020-08-30 at 05 49 15

@WoLewicki
Copy link
Member

Looks like it is the same issue as #564. Am I right?

@dotconnor
Copy link
Author

Just tested this, and the issue still happens regardless of the View inside the screen, secondly it also happens regardless if the translucent header option is set or not.

@WoLewicki
Copy link
Member

There seem to be some layout issues when having a UIViewController (from ScreenContainer of bottom-tabs) between 2 UINavigationControllers (from ScreenStack of native-stack). As a workaround, you can make the top navigator a normal stack (since you don't use any of the native-stack specific options, right?).

@ArekChr
Copy link

ArekChr commented Sep 9, 2020

There seem to be some layout issues when having a UIViewController (from ScreenContainer of bottom-tabs) between 2 UINavigationControllers (from ScreenStack of native-stack). As a workaround, you can make the top navigator a normal stack (since you don't use any of the native-stack specific options, right?).

I'm using large and blurred headers from native-stack

@WoLewicki
Copy link
Member

WoLewicki commented Sep 9, 2020

@ArekChr It is not a solution to the bug, but a workaround, and I know it cannot be applied in many cases.

@burakgormek
Copy link
Contributor

@WoLewicki Any update? I have same issue. I just noticed that this is only happens if stack has animation.
stackAnimation 'none' works without an issue.

simplified test: https://snack.expo.io/KrD6WIJ2l (looks like iOS only)

@WoLewicki
Copy link
Member

@burakgormek @ArekChr @dotconnor can you check if #1025 fixes the issue and does not introduce any new layout issues? I described the scenario where it won't work, but hopefully it is not common enough nesting pattern.

@burakgormek
Copy link
Contributor

burakgormek commented Jul 22, 2021

@WoLewicki
Thanks for the PR.

I've tested with the PR and it fixes header jump but only for vertical. Horizontal jumps looks still there.
simplified test: https://snack.expo.dev/IYo_xZr5a

Not know if this is another issue or not connected this issue though.

Container
    Screen: Content
    Screen: SecondStack // Causing the problem. Works normally without this stack.
            Screen: Bottom Tab
                Screen: ChildStack > Content

With SecondStack:

screen1

Without SecondStack:

screen2

Screenshots saved before the navigate transition ends.

With PR vertical jumps gone but horizontal still there:

RPReplay_Final1626991190.mp4

@WoLewicki
Copy link
Member

@burakgormek I tested the snack you provided and it seems that unfortunately it is a different problem than the one fixed by the PR and will not be easy to resolve. Could you submit another issue with suitable description and this reproduction and we will close this one as #1025 is merged?

@WoLewicki
Copy link
Member

WoLewicki commented Jul 28, 2021

I came up with another option of how to resolve these issues, which is making ScreenContainer a UINavigationController for bottom-tabs and drawer (#1029). It is not easy/possible for stack navigator. Can you check if it resolves your issues and does not introduce new ones? It requires applying PR from react-navigation for it to work: react-navigation/react-navigation#9772.

@WoLewicki WoLewicki linked a pull request Jul 28, 2021 that will close this issue
3 tasks
@burakgormek
Copy link
Contributor

burakgormek commented Jul 28, 2021

@WoLewicki
I tested the latest PR and can confirm that it works perfect! Thanks.
Also I didn't see any layout issues on my app.

Btw I think you linked the wrong/old one. I tested #1029

Final result:

RPReplay_Final1627492103.mp4

@WoLewicki
Copy link
Member

Yeah, I meant #1029, sorry. 😅

@swrobel
Copy link

swrobel commented Aug 4, 2021

I have the same issue demonstrated in the original post on this issue when using nested native stack navigators. I tested both of the following:

@WoLewicki
Copy link
Member

@swrobel can you post a reproduction where #1029 does not fix the jumping issue?

@SimpleCreations
Copy link

@WoLewicki I'll throw in my two cents. I have the same issue with a native stack navigator nested in a tab navigator nested in another native stack navigator.
I have the same results as @swrobel - #1025 fixes the issue but #1029 doesn't. Here's the repro.

import React from "react";
import { Button, SafeAreaView } from "react-native";
import { NavigationContainer } from "@react-navigation/native";
import { createNativeStackNavigator, NativeStackScreenProps } from "react-native-screens/native-stack";
import { createBottomTabNavigator } from "@react-navigation/bottom-tabs";

const RootStack = createNativeStackNavigator();
const BottomTabs = createBottomTabNavigator();
const ChildStack = createNativeStackNavigator();

type RootStackParamsList = {
  BottomTabsScreen: undefined;
};

const HomeScreen = ({navigation}: NativeStackScreenProps<RootStackParamsList>) => (
  <SafeAreaView>
    <Button
      title="Navigate to BottomTabsScreen"
      onPress={() => navigation.navigate("BottomTabsScreen")} />
  </SafeAreaView>
);

const BottomTabsScreen = () => (
  <BottomTabs.Navigator
    initialRouteName="FirstBottomTabScreen"
    screenOptions={{headerShown: false}}>
    <BottomTabs.Screen
      name="FirstBottomTabScreen"
      component={FirstBottomTabScreen} />
  </BottomTabs.Navigator>
);

const FirstBottomTabScreen = () => (
  <ChildStack.Navigator
    initialRouteName="ChildStackScreen">
    <ChildStack.Screen
      name="ChildStackScreen"
      component={ChildStackScreen} />
  </ChildStack.Navigator>
);

const ChildStackScreen = () => null;

const App = () => (
  <NavigationContainer>
    <RootStack.Navigator
      initialRouteName="HomeScreen"
      screenOptions={{headerShown: false}}>
      <RootStack.Screen
        name="HomeScreen"
        component={HomeScreen} />
      <RootStack.Screen
        name="BottomTabsScreen"
        component={BottomTabsScreen} />
    </RootStack.Navigator>
  </NavigationContainer>
);

export default App;

@burakgormek
Copy link
Contributor

@SimpleCreations, @swrobel

Did you patch react-navigation PR with #1029?
react-navigation/react-navigation#9772

@swrobel
Copy link

swrobel commented Aug 4, 2021

Did you patch react-navigation PR with #1029?
react-navigation/react-navigation#9772

I did. It was a bit tricky to figure out how to do it in package.json, but this seemed to work for me:
"@react-navigation/bottom-tabs": "https://gitpkg.now.sh/react-navigation/react-navigation/packages/bottom-tabs?@wolewicki/add-tabs-or-drawer-prop"

I didn't need to make any other changes to my code, correct?

@SimpleCreations
Copy link

@burakgormek no, but after having done that, the issue is gone as well. To clarify, I installed #1029 and #9772 from react-navigation.

@SimpleCreations
Copy link

@swrobel this worked for me:

npm i "https://gitpkg.now.sh/react-navigation/react-navigation/packages/bottom-tabs?8cc35e349fa767a8b9c613f2ec28e8c5b0cd64ea

@WoLewicki
Copy link
Member

I think it would be easiest to just change it in your node_modules for now. You can use patch-package if you want to have all those changes in the code. So I believe #1029 with the PR from react-navigation fixes the problem yeah? @SimpleCreations @swrobel

@SimpleCreations
Copy link

So I believe #1029 with the PR from react-navigation fixes the problem yeah? @SimpleCreations @swrobel

Yes

@swrobel
Copy link

swrobel commented Aug 4, 2021

Not for me, although I will re-test & post a repro if I'm able to create one outside of my project. I had to upgrade to react-navigation 6.x as part of this testing, so I need to make sure I did that correctly. There are a lot of upgrade notes...

@WoLewicki
Copy link
Member

@swrobel keep in mind that it won't work if you have a stack navigator between the native-stacks, but we discourage nesting navigators of same kind one in another.

@swrobel
Copy link

swrobel commented Aug 4, 2021

@swrobel keep in mind that it won't work if you have a stack navigator between the native-stacks, but we discourage nesting navigators of same kind one in another.

Do you mean a non-native stack navigator nested inside a native-stack? I don't have that setup, but I do have nested native-stack navigators.

@a-eid
Copy link

a-eid commented Aug 5, 2021

having a similar issue.

header.issue.mp4

@WoLewicki
Copy link
Member

@a-eid it still happens after incorporating both PRs mentioned in #619 (comment)? @swrobel I meant the first scenario, so it should be fine. Please provide a repo with reproduction then, would be best if it already had the needed changes e.g. by using patch-package.

@a-eid
Copy link

a-eid commented Sep 9, 2021

@WoLewicki issue still happening with the latest release 3.7.0.

@burakgormek
Copy link
Contributor

@a-eid You have to patch react-navigation PR which is not merged yet.
react-navigation/react-navigation#9772

@a-eid
Copy link

a-eid commented Sep 9, 2021

@burakgormek thanks alot, I confirm that patching react navigation has fixed the issue for me.

cc @WoLewicki

@vesparny
Copy link

@a-eid how did you patch it?

@a-eid
Copy link

a-eid commented Sep 20, 2021

I only needed to patch the bottom tab lib, also you need to be using version 6.

you need to open the package inside node_modules and changes the files accordingly as per the PR above.

patches/@react-navigation+bottom-tabs+6.0.5.patch

diff --git a/node_modules/@react-navigation/bottom-tabs/src/views/BottomTabView.tsx b/node_modules/@react-navigation/bottom-tabs/src/views/BottomTabView.tsx
index 63a0895..32fa437 100644
--- a/node_modules/@react-navigation/bottom-tabs/src/views/BottomTabView.tsx
+++ b/node_modules/@react-navigation/bottom-tabs/src/views/BottomTabView.tsx
@@ -92,6 +92,7 @@ export default function BottomTabView(props: Props) {
     <SafeAreaProviderCompat>
       <MaybeScreenContainer
         enabled={detachInactiveScreens}
+        hasTwoStates
         style={styles.container}
       >
         {routes.map((route, index) => {
diff --git a/node_modules/@react-navigation/bottom-tabs/src/views/ScreenFallback.tsx b/node_modules/@react-navigation/bottom-tabs/src/views/ScreenFallback.tsx
index 36789cf..f4bb975 100644
--- a/node_modules/@react-navigation/bottom-tabs/src/views/ScreenFallback.tsx
+++ b/node_modules/@react-navigation/bottom-tabs/src/views/ScreenFallback.tsx
@@ -22,6 +22,7 @@ export const MaybeScreenContainer = ({
   ...rest
 }: ViewProps & {
   enabled: boolean;
+  hasTwoStates: boolean;
   children: React.ReactNode;
 }) => {
   if (Screens?.screensEnabled?.()) {

this applies changes to the bottom-tabs only, if you're using a drawer you'd need to edit the package files as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants