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

formSheet presentation only opening half-way in 3.19.0 #1686

Closed
gorbypark opened this issue Jan 19, 2023 · 17 comments
Closed

formSheet presentation only opening half-way in 3.19.0 #1686

gorbypark opened this issue Jan 19, 2023 · 17 comments
Assignees
Labels
Missing info The user didn't precise the problem enough Missing repro This issue need minimum repro scenario Platform: iOS This issue is specific to iOS

Comments

@gorbypark
Copy link
Contributor

Description

formSheet presentation screens are only opening half-way in 3.19.0. I reverted to 3.18.2 and everything works as normal. Looking through the change logs I'm not seeing anything obvious to the cause of this. Originally I had thought the new formSheet detent PR had merged/released, but it hasn't yet #1649 (comment). I have only tried on the "old architecture". The two screenshots below show 3.19.0 and 3.18.2, respectively, with no other changes made besides the version.

3.19.0
Screen Shot 3.19.0

3.18.2
Screen Shot 3.18.2

Steps to reproduce

  1. Upgrade to react-native-screens 3.19.0
  2. set 'presentation: 'formSheet' on a screen

Snack or a link to a repository

.

Screens version

3.19.0

React Native version

0.71.0

Platforms

iOS

JavaScript runtime

None

Workflow

None

Architecture

Paper (Old Architecture)

Build type

Debug mode

Device

iOS simulator

Device model

No response

Acknowledgements

Yes

@github-actions github-actions bot added the Missing repro This issue need minimum repro scenario label Jan 19, 2023
@github-actions
Copy link

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

@github-actions github-actions bot added Platform: iOS This issue is specific to iOS Missing info The user didn't precise the problem enough labels Jan 19, 2023
@github-actions
Copy link

Hey! 👋

It looks like you've omitted a few important sections from the issue template.

Please complete Snack or a link to a repository section.

@gorbypark
Copy link
Contributor Author

gorbypark commented Jan 19, 2023

After further investigation, it appears that the medium detent PR was actually merged in 3.19.0? 3.18.2...3.19.0 the changelog is showing it as included in 3.19.0. It also looks there was a PR e20f54c that removed the props for controlling the detents, so my best guess is the default size is somehow set to medium/half, but we have no way of controlling it since the props were temporarily removed until the feature is ready for prime time.

@kkafar
Copy link
Member

kkafar commented Jan 19, 2023

Yeah, this might be a my mistake, as I've left some part of native implementation intact... But while testing I didn't notice the change of default behaviour. I'll check this & let you know

@kkafar
Copy link
Member

kkafar commented Jan 21, 2023

Yep, everything works fine on main, I've made mistake when removing public API and wrong default value is passed in 3.19.0 🤦🏻‍♂️

It will be fixed with next minor version (won't give you details, but it should not be long from now).

@kkafar kkafar closed this as completed Jan 21, 2023
@kkafar kkafar self-assigned this Jan 21, 2023
@SMJ93
Copy link

SMJ93 commented Feb 2, 2023

Hey @kkafar, when do you plan to release 3.19.1?

@hirbod
Copy link
Contributor

hirbod commented Feb 11, 2023

@kkafar this issue has not been resolved yet as we're still on 3.19 and Expo SDK 48 is about to ship with 3.19.0. Could we get a 3.19.1 to be shipped with Expo SDK 48 before its too late? Thanks!

@kkafar
Copy link
Member

kkafar commented Feb 13, 2023

Should be fixed with: https://github.com/software-mansion/react-native-screens/releases/tag/3.20.0

Edit: let me know whether it works fine again also on your side

@adamivancza
Copy link

adamivancza commented Jul 5, 2023

@kkafar the issue was fixed in 3.20.0 but was re-introduced again in later versions. just hit this with 3.22.0.

@kkafar
Copy link
Member

kkafar commented Jul 6, 2023

Hey @adamivancza, I've just tested it on main -- seems to me that sheet presentation works just fine.
Here's my test case:

import * as React from 'react';
import {Button, StyleSheet, View, Text, ScrollView} from 'react-native';
import {NavigationContainer, ParamListBase} from '@react-navigation/native';
import {
  createNativeStackNavigator,
  NativeStackNavigationProp,
} from 'react-native-screens/native-stack';
import {SheetDetentTypes} from 'react-native-screens';

const Stack = createNativeStackNavigator();

export default function App(): JSX.Element {
  return (
    <NavigationContainer>
      <Stack.Navigator
        screenOptions={{
          // headerRight: () => <View style={styles.headerView} />,
          headerTitleStyle: {
            color: 'cyan',
          },
          headerShown: true,
          headerHideBackButton: false,
        }}>
        <Stack.Screen name="First" component={First} />
        <Stack.Screen
          name="SheetScreen"
          component={SheetScreen}
          options={{
            stackPresentation: 'formSheet',
          }}
        />
      </Stack.Navigator>
    </NavigationContainer>
  );
}

function First({
  navigation,
}: {
  navigation: NativeStackNavigationProp<ParamListBase>;
}) {
  return (
    <Button
      title="Tap me for the second screen"
      onPress={() => navigation.navigate('SheetScreen')}
    />
  );
}

function SheetScreen({
  navigation,
}: {
  navigation: NativeStackNavigationProp<ParamListBase>;
}) {
  return (
    <View style={styles.centeredView}>
      <Button
        title="Tap me for the first screen"
        onPress={() => navigation.navigate('First')}
      />
      <Button
        title="Tap me for the second screen"
        onPress={() => navigation.navigate('Second')}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  headerView: {
    height: 20,
    width: 20,
    backgroundColor: 'red',
  },
  centeredView: {
    justifyContent: 'center',
    alignItems: 'center',
  },
});

The sheet opens by default to full height.

Provide me with repro / give more detail please.

@adamivancza
Copy link

hey @kkafar! Maybe you're using something outdated in your repro as stackPresentation was renamed to presentation?
https://reactnavigation.org/docs/native-stack-navigator/#presentation
https://reactnavigation.org/docs/upgrading-from-5.x/#options-of-native-stack

@kkafar
Copy link
Member

kkafar commented Jul 6, 2023

@adamivancza, I see now.
Difference in prop naming comes from the fact that I'm using only react-native-screens.
Please note that I'm importing native stack directly from react-native-screens not from @react-navigation/native-stack (which uses react-native-screens components under the hood).
I'll check out whether it works with @react-navigation & let you know whether the issue should be moved to their repo.

@adamivancza
Copy link

Screenshot 2023-07-06 at 12 32 58

will attach a full repro project shortly

@adamivancza
Copy link

full repro project:
formsheet-repro.zip

@kkafar
Copy link
Member

kkafar commented Jul 6, 2023

@adamivancza,
could you try out react-native-screens from this branch #1811 ?

You can put the following in your package.json:

"react-native-screens": "software-mansion/react-native-screens#@kkafar/check-sheet-presentation-on-ios"

Let me know whether the fix works.

kkafar added a commit that referenced this issue Jul 6, 2023
…nnerScreen` (#1811)

## Description

Default values for medium-detent related props were set only in
`NativeStackView` so only in case user used native stack directly from
`react-native-screens`. This led to buggy behaviour when
`@react-navigation/native-stack` was in use.

See conversation:
#1686 (comment)

## Changes

Default prop values are now set in `InnerScreen` in `index.native.tsx`
which is used internally by `@react-navigation/native-stack` package.

# Test case

See conversation:
#1686 (comment)
(and below).

Use `stackPresentation: 'formSheet'` with native stack from
`@react-navigation/native-stack` and see that the modal is opened by
default to full height.

## Checklist

- [x] Ensured that CI passes
@adamivancza
Copy link

adamivancza commented Jul 6, 2023

yeah, I can confirm that works fine @kkafar! thx for the quick fix 😉
can you release this fix as a patch?

Screenshot 2023-07-06 at 14 09 04

@kkafar
Copy link
Member

kkafar commented Jul 6, 2023

@adamivancza It's already released (should be avaiable directly from NPM)

mccoyplayer pushed a commit to mccoyplayer/reactScreen that referenced this issue Feb 9, 2024
…nnerScreen` (#1811)

## Description

Default values for medium-detent related props were set only in
`NativeStackView` so only in case user used native stack directly from
`react-native-screens`. This led to buggy behaviour when
`@react-navigation/native-stack` was in use.

See conversation:
software-mansion/react-native-screens#1686 (comment)

## Changes

Default prop values are now set in `InnerScreen` in `index.native.tsx`
which is used internally by `@react-navigation/native-stack` package.

# Test case

See conversation:
software-mansion/react-native-screens#1686 (comment)
(and below).

Use `stackPresentation: 'formSheet'` with native stack from
`@react-navigation/native-stack` and see that the modal is opened by
default to full height.

## Checklist

- [x] Ensured that CI passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing info The user didn't precise the problem enough Missing repro This issue need minimum repro scenario Platform: iOS This issue is specific to iOS
Projects
None yet
Development

No branches or pull requests

5 participants