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): update HeaderConfig view controller after unmounting subviews #2230

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jul 4, 2024

Description

When using a header left view and then removing it the default back button doesn't reappear on new arch. This is because we do not call updateViewControllerIfNeeded when removing views.

Changes

Call updateViewControllerIfNeeded in unmountChildComponentView.

Test code and steps to reproduce

In the FabricExample HeaderOptions example:

  • Open Header item setting
  • Choose left
  • Choose right
  • See the back button is gone.
image image image

When toggling state the default back button doesn't reappear.

Checklist

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

@kkafar kkafar requested review from tboba and kkafar and removed request for tboba July 29, 2024 10:42
@tboba
Copy link
Member

tboba commented Jul 29, 2024

Hi @janicduplessis, thanks for submitting this PR!
Nice catch, indeed back button is not rendered (left subview is not being unmounted aswell), when we're trying to remove left subview of the navigation bar.

Let me just pass my reproducer for that specific issue below:

Reproducer
import React from 'react';
import { View, StyleSheet } from 'react-native';
import {
  createNativeStackNavigator,
  NativeStackNavigationProp,
} from '@react-navigation/native-stack';
import { Button } from '../shared';

type StackParamList = {
  Main: undefined;
  Detail: undefined;
};

interface MainScreenProps {
  navigation: NativeStackNavigationProp<StackParamList, 'Main'>;
}

const MainScreen = ({ navigation }: MainScreenProps): React.JSX.Element => (
  <View style={{ ...styles.container, backgroundColor: 'moccasin' }}>
    <Button
      testID="simple-native-stack-go-to-detail"
      title="Go to detail"
      onPress={() => navigation.navigate('Detail')}
    />
    <Button onPress={() => navigation.pop()} title="🔙 Back to Examples" />
  </View>
);

interface DetailScreenProps {
  navigation: NativeStackNavigationProp<StackParamList, 'Detail'>;
}

const DetailScreen = ({ navigation }: DetailScreenProps): React.JSX.Element => {
  const [backButtonik, setBackButtonik] = React.useState(false);

  return (
    <View style={{ ...styles.container, backgroundColor: 'thistle' }}>
      <Button
        title="Go back"
        onPress={() => navigation.goBack()}
        testID="simple-native-stack-detail-go-back"
      />
      <Button
        title="Switch"
        onPress={() => {
          setBackButtonik(prev => !prev);
          navigation.setOptions({
            // headerBackVisible: !backButtonik,
            headerLeft: backButtonik
              ? () => (
                  <View
                    style={{ width: 20, height: 20, backgroundColor: 'green' }}
                  />
                )
              : undefined,
          });
        }}
      />
    </View>
  );
};

const Stack = createNativeStackNavigator<StackParamList>();

interface Props {
  color?: string;
  size?: number;
}

const Square = ({ size = 100, color = 'red' }: Props): React.JSX.Element => (
  <View style={{ width: size, height: size, backgroundColor: color }} />
);

const App = (): React.JSX.Element => (
  <Stack.Navigator
    screenOptions={
      {
        // headerBackVisible: false,
      }
    }>
    <Stack.Screen
      name="Main"
      component={MainScreen}
      options={{ title: 'Simple Native Stack' }}
    />
    <Stack.Screen
      name="Detail"
      component={DetailScreen}
      options={{
        headerLeft: () => <Square color="green" size={20} />,
      }}
    />
  </Stack.Navigator>
);

const styles = StyleSheet.create({
  container: {
    flex: 1,
    paddingTop: 10,
  },
});

export default App;

Thanks!

@tboba tboba changed the title Fix default back button not appearing when removing header left fix(iOS): Update HeaderConfig view controller after unmounting subviews Jul 29, 2024
@tboba tboba changed the title fix(iOS): Update HeaderConfig view controller after unmounting subviews fix(iOS): update HeaderConfig view controller after unmounting subviews Jul 29, 2024
@tboba tboba merged commit 5f9f061 into software-mansion:main Jul 29, 2024
5 checks passed
@janicduplessis janicduplessis deleted the patch-8 branch July 29, 2024 14:58
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…ws (software-mansion#2230)

## Description

When using a header left view and then removing it the default back
button doesn't reappear on new arch. This is because we do not call
`updateViewControllerIfNeeded` when removing views.

## Changes

Call `updateViewControllerIfNeeded` in `unmountChildComponentView`.

## Test code and steps to reproduce

In the FabricExample HeaderOptions example:

- Open Header item setting
- Choose left
- Choose right
- See the back button is gone.

<img width="368" alt="image"
src="https://github.com/software-mansion/react-native-screens/assets/2677334/d5bfea6e-d0f4-4527-9f90-99f19c6045e5">

<img width="381" alt="image"
src="https://github.com/software-mansion/react-native-screens/assets/2677334/3338435c-c48f-4858-bd8a-97103a396fcd">

<img width="368" alt="image"
src="https://github.com/software-mansion/react-native-screens/assets/2677334/143349b0-6f66-460b-9920-14e4a50f16bc">

When toggling state the default back button doesn't reappear.

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
kkafar added a commit that referenced this pull request Oct 15, 2024
## Description

The `updateViewControllerIfNeeded` call introduced by
#2230
forces the view controller to rebuild the subviews with the recent
config.
When the screen is being unmounted it replaces the subviews with nil
values as the `reactSubviews` are removed from the config one by one.
Snapshots made earlier get discarded in the process.

This PR adds a condition that prevents updating the view controller when
the screen is being changed + stops unnecessary snapshots when the
screen is not changed.

## Changes

- updated `Test556.tsx` repro
- added `isGoingToBeRemoved` property to `RNSScreenView`
- making snapshots / updating the view controller conditionally

 <!--
## Screenshots / GIFs
-->

## Test code and steps to reproduce

- Use `Test556.tsx` repro

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
kkafar pushed a commit that referenced this pull request Oct 25, 2024
The `updateViewControllerIfNeeded` call introduced by
#2230
forces the view controller to rebuild the subviews with the recent
config.
When the screen is being unmounted it replaces the subviews with nil
values as the `reactSubviews` are removed from the config one by one.
Snapshots made earlier get discarded in the process.

This PR adds a condition that prevents updating the view controller when
the screen is being changed + stops unnecessary snapshots when the
screen is not changed.

- updated `Test556.tsx` repro
- added `isGoingToBeRemoved` property to `RNSScreenView`
- making snapshots / updating the view controller conditionally

 <!--
-->

- Use `Test556.tsx` repro

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
(cherry picked from commit e4333a1)
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