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(iOS)!: Add system/systemInverted prop for statusBarStyle #2106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tboba
Copy link
Member

@tboba tboba commented Apr 18, 2024

Description

This PR intents to fix an issue, where headers were not styled correctly in react-navigation. It adds two new props: system and systemInverted that will be used for setting system-based theme of status bar. These were previously in screens, but were not used by default, since we were mostly relying on react-navigation's dark theme. As we still want to rely on the theme, configured in NavigationContainer, this PR also adds new auto prop, that will use dark setting from useTheme hook to control, whether there should be light or dark theme set.

Note: this change is meant to be breaking, since it changes default behavior of statusBarStyle and removes inverted prop (replaced by systemInverted).

Fixes #2083, react-navigation/react-navigation#11397

Changes

  • Changed the name of auto and inverted props to system and systemInverted
  • Added new functionality for auto prop for setting theme, according to the one from react-navigation.

Screenshots / GIFs

8mb.video-okl-CHWyXdN5.mp4

I've managed to compress 335mb into 6mb - I'd recommend watching this on fullscreen mode 😨

Test code and steps to reproduce

I've mostly edited Test2069.tsx to add a theme for NavigationContainer 👇

Code preview
import {
  NavigationContainer,
  DefaultTheme,
  DarkTheme,
} from '@react-navigation/native';
import {
  createNativeStackNavigator,
  NativeStackNavigationProp,
} from '@react-navigation/native-stack';
import React, { useState } from 'react';
import { StyleSheet, Text, TouchableOpacity, View } from 'react-native';

type StackParamList = {
  Home: undefined;
  Home1: undefined;
  Home2: undefined;
  Home3: undefined;
};

interface MainScreenProps {
  navigation: NativeStackNavigationProp<StackParamList>;
}

const Home = ({ navigation }: MainScreenProps) => (
  <View style={styles.view}>
    <Text
      onPress={() => {
        navigation.navigate('Home1');
        navigation.navigate('Home2');
      }}>
      This is the initial View
    </Text>
  </View>
);

const Home1 = ({ navigation }: MainScreenProps) => (
  <View style={styles.view}>
    <Text onPress={() => navigation.goBack()}>This is View 1</Text>
  </View>
);

const Home2 = ({ navigation }: MainScreenProps) => (
  <View style={styles.view}>
    <Text onPress={() => navigation.goBack()}>This is View 2</Text>
  </View>
);

const Home3 = ({ navigation }: MainScreenProps) => (
  <View style={styles.view}>
    {/* goBack shouldn't work here. */}
    <Text onPress={() => navigation.goBack()}>This is View 3</Text>
  </View>
);

const Stack = createNativeStackNavigator();

const Test2069 = () => {
  const [hasChangedState, setHasChangedState] = useState(0);

  const MyTheme = {
    ...DefaultTheme,
    // ...DarkTheme,
    dark: false,
  };

  return (
    <NavigationContainer theme={MyTheme}>
      <Stack.Navigator screenOptions={{ statusBarStyle: 'auto' }}>
        {hasChangedState % 3 === 0 ? (
          <>
            <Stack.Screen name="Home" component={Home} />
            <Stack.Screen name="Home1" component={Home1} />
            <Stack.Screen name="Home2" component={Home2} />
          </>
        ) : hasChangedState % 3 === 1 ? (
          <>
            <Stack.Screen name="Home2" component={Home2} />
          </>
        ) : (
          <>
            <Stack.Screen name="Home3" component={Home3} />
          </>
        )}
      </Stack.Navigator>
      <TouchableOpacity
        style={styles.button}
        onPress={() => setHasChangedState(old => old + 1)}>
        <Text>Change state</Text>
      </TouchableOpacity>
    </NavigationContainer>
  );
};

const styles = StyleSheet.create({
  button: {
    justifyContent: 'center',
    alignItems: 'center',
    height: 100,
  },
  view: {
    alignItems: 'center',
    backgroundColor: '#b7c4bb',
    flex: 1,
    justifyContent: 'center',
    padding: 12,
  },
});

export default Test2069;

Checklist

  • Included code example that can be used to test this change
  • Updated TS types
  • Ensured that CI passes

@tboba tboba changed the title [🔨] fix(iOS): Add system/systemInverted prop for statusBarStyle [💣] fix(iOS): Add system/systemInverted prop for statusBarStyle Apr 18, 2024
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

Left a few comments, if it is going to be introduced in new version, we can get rid of the previous values totally.

@@ -268,6 +268,10 @@ const RouteView = ({
const Screen = React.useContext(ScreenContext);
const { dark } = useTheme();

if (statusBarStyle === 'auto') {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some warning that the behavior changed and you should use system? Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think adding some note in the changelog should be sufficient - especially, if we want to merge this for v4 😄

@@ -44,11 +44,11 @@ + (UIStatusBarStyle)statusBarStyleForRNSStatusBarStyle:(RNSStatusBarStyle)status
__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_13_0
if (@available(iOS 13.0, *)) {
switch (statusBarStyle) {
case RNSStatusBarStyleAuto:
case RNSStatusBarStyleSystem:
Copy link
Member

Choose a reason for hiding this comment

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

What if someone passes RNSStatusBarStyleAuto now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, since we're not controlling the behavior of auto prop on the native side (it's handled on JS with the dark prop from useTheme()), I believe we don't need to handle the case of auto here.

*/
statusBarStyle?: 'inverted' | 'auto' | 'light' | 'dark';
statusBarStyle?: 'auto' | 'system' | 'systemInverted' | 'light' | 'dark';
Copy link
Member

Choose a reason for hiding this comment

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

So now inverted and auto will not be available at all? If so, I guess it will land in v4?

Copy link
Member Author

@tboba tboba Apr 19, 2024

Choose a reason for hiding this comment

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

Most likely. I was thinking if we want to keep inverted option that will be reverse of auto (or just keep it as a fallback option to systemInverted), but I'm for introducing this change on v4, yeah.

@kkafar kkafar self-requested a review April 27, 2024 17:56
@tboba tboba changed the title [💣] fix(iOS): Add system/systemInverted prop for statusBarStyle fix(iOS)!: Add system/systemInverted prop for statusBarStyle May 6, 2024
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.

iOS Status Bar Error
2 participants