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

fix(Android): modal insets not applying #2371

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Oct 2, 2024

Description

This PR fixes the insets not applying on modal screens.
I brought back the missing logic, that was removed here to InsetsObserverProxy

Note: Without this change the insets are applied correctly after navigating to a screen with presentation: formSheet at least once.

Changes

  • removed unused unregister fun from InsetsObserverProxy
  • added unregisterOnView that resembles the previously removed logic

Screenshots / GIFs

Before

Screenshot 2024-10-02 at 13 07 51

After

Screenshot 2024-10-02 at 13 54 15

Test code and steps to reproduce

  • use Stack Presentation in the Example app
  • set headerShown: false for the modal screen
  • open the modal screen

Checklist

  • Ensured that CI passes

@alduzy alduzy requested a review from kkafar October 2, 2024 13:38
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look promising, however let's make sure we test this change on various stackPresentation & combinations. Also it might be important to test this in bottom-tabs environment.

Please confirm if you have tested these cases.

@alduzy
Copy link
Member Author

alduzy commented Oct 2, 2024

@kkafar tested it with Example app's "Stack Presentation" and "Bottom tabs and native stack" + some test cases and can confirm it works as expected.

@alduzy alduzy requested a review from kkafar October 2, 2024 14:39
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still worried, that removing InsetObserverProxy from the decor view might interfere with formSheet presentation.

Imagine case where you have tab navigator, each screen is a native stack, you present formSheet in one of them & open keyboard. I see it not working. I'll try to test it as well before merging.

@alduzy
Copy link
Member Author

alduzy commented Oct 3, 2024

@kkafar I prepared a snack that can be used to test that:

snack
import { createBottomTabNavigator } from '@react-navigation/bottom-tabs';
import { NavigationContainer } from '@react-navigation/native';
import {
  createNativeStackNavigator,
  NativeStackNavigationProp,
} from '@react-navigation/native-stack';
import React from 'react';
import { Button, StyleSheet, Text, TextInput, View } from 'react-native';

type StackParams = {
  Home: undefined;
  Sheet: undefined;
  Modal: undefined;
  Tab?: undefined;
};

const Stack = createNativeStackNavigator();
const Tab = createBottomTabNavigator();
const InnerStack = createNativeStackNavigator<StackParams>();

function HomeScreen({
  navigation,
}: {
  navigation: NativeStackNavigationProp<StackParams>;
}) {
  return (
    <View style={styles.container}>
      <Button title="Open sheet" onPress={() => navigation.navigate('Sheet')} />
      <Button title="Open modal" onPress={() => navigation.navigate('Modal')} />
    </View>
  );
}

function InputScreen({
  navigation,
}: {
  navigation: NativeStackNavigationProp<StackParams>;
}) {
  return (
    <View style={styles.container}>
      <Text style={styles.heading}>This is a screen</Text>
      <TextInput placeholder="Input something" />
      <Button title="Dismiss" onPress={() => navigation.popTo('Home')} />
    </View>
  );
}

function InnerStackNavigator() {
  return (
    <InnerStack.Navigator>
      <InnerStack.Screen name="Home" component={HomeScreen} />
      <InnerStack.Screen
        name="Modal"
        component={InputScreen}
        options={{ presentation: 'modal' }}
      />
      <InnerStack.Screen
        name="Sheet"
        component={InputScreen}
        options={{ presentation: 'formSheet' }}
      />
    </InnerStack.Navigator>
  );
}

function TabNavigator() {
  return (
    <Tab.Navigator>
      <Tab.Screen name="Inner Stack" component={InnerStackNavigator} />
    </Tab.Navigator>
  );
}

export default function App() {
  return (
    <NavigationContainer>
      <Stack.Navigator>
        <Stack.Screen name="Tab" component={TabNavigator} />
      </Stack.Navigator>
    </NavigationContainer>
  );
}

const styles = StyleSheet.create({
  heading: {
    fontSize: 16,
    fontWeight: 'bold',
    color: 'black',
    marginBottom: 16,
    textAlign: 'center',
  },
  container: {
    flex: 1,
    padding: 10,
    backgroundColor: 'mediumseagreen',
  },
});

@alduzy alduzy force-pushed the @alduzy/android-modals-status-bar-fix branch from 9758d45 to f61d608 Compare October 9, 2024 09:22
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quickly tested this, did not saw any regression, however this is sensitive code, which I do not understand completely.

We need to do some deeper research on inset management in our apps.

@alduzy alduzy merged commit 6154c2b into main Oct 10, 2024
4 checks passed
@alduzy alduzy deleted the @alduzy/android-modals-status-bar-fix branch October 10, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants