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): change RNSScreenDetentType enum to work correctly on Fabric #1964

Closed
wants to merge 1 commit into from

Conversation

aljs
Copy link

@aljs aljs commented Nov 9, 2023

Description

When opening a formSheet (on iOS) from @react-navigation/native-stack on new arch I've experienced the same issue as #1686 . After some debugging I've discovered that sheetAllowedDetents={'large'} transfers to RNSScreenDetentTypeMedium and sheetAllowedDetents={'medium'} to RNSScreenDetentTypeLarge. This PR fixes it.

Changes

Updated ios/RNSEnums.h

Test code and steps to reproduce

Checklist

@kkafar
Copy link
Member

kkafar commented Nov 9, 2023

Hey @aljs, thanks for taking time for investigation and helping us out! However, I doubt that this is the real source of the issue. Most likely this fails because the feature has not been released & tested with react-navigation. I mean, it is possible to use it, as the props passed via react-navigation components fall through to react-native-screens components however there is some JS logic missing on side of react-navigation. I'll put it into my backlog! Thanks again!

@tboba tboba changed the title fix RNSScreenDetentType enum to work correctly on new RN arch fix(iOS): change RNSScreenDetentType enum to work correctly on Fabric Nov 9, 2023
@kkafar
Copy link
Member

kkafar commented Nov 16, 2023

Yep, I've not merged iOS detents to react-navigation yet, there is JS logic missing there.
Thank you for contribution, I'll handle this in @react-navigation repo.

@kkafar kkafar closed this Nov 16, 2023
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