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

feat(iOS): sheetInitialDetent support #2367

Merged
merged 10 commits into from
Oct 2, 2024
Merged

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Oct 1, 2024

Description

This PR adds sheetInitialDetent support to iOS and fixes detents logic on iOS < 16.

This functionality depends on changes made in this PR: react-navigation/react-navigation#12032

Changes

  • supporting sheetInitialDetent on iOS
  • modified Test2002.tsx repro
  • updated README definition to match types
  • fixed sheetAllowedDetents on iOS < 16

Test code and steps to reproduce

  • Use Test2002.tsx repro

Checklist

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.

Looks solid, good job

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've not tested the runtime though, make sure it works as expected & we're good.

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 gave premature approval - have one question

ios/RNSScreen.mm Outdated Show resolved Hide resolved
@alduzy
Copy link
Member Author

alduzy commented Oct 2, 2024

@kkafar I updated the PR. Tested on iOS 15.5 and iOS 18 successfully

@alduzy alduzy requested a review from kkafar October 2, 2024 09:27
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.

Just a question

Comment on lines +892 to +893
if (@available(iOS 16.0, *)) {
if (_sheetAllowedDetents.count > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this swap done?

Copy link
Member Author

Choose a reason for hiding this comment

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

_sheetAllowedDetents.count > 0 is true in most cases. Nothing would happen on iOS < 16 as such case isn't handled inside the condition. After swapping them both versions work correctly

Copy link
Member

Choose a reason for hiding this comment

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

okay, thanks!

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.

Looks solid 💪🏻

Comment on lines +892 to +893
if (@available(iOS 16.0, *)) {
if (_sheetAllowedDetents.count > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

okay, thanks!

@alduzy alduzy merged commit 606cd4b into main Oct 2, 2024
8 checks passed
@alduzy alduzy deleted the @alduzy/ios-sheet-initial-detent branch October 2, 2024 10:38
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
## Description

This PR adds `sheetInitialDetent` support to iOS and fixes detents logic
on iOS < 16.

This functionality depends on changes made in this PR:
react-navigation/react-navigation#12032

## Changes

- supporting `sheetInitialDetent` on iOS
- modified `Test2002.tsx` repro
- updated README definition to match types
- fixed `sheetAllowedDetents` on iOS < 16

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

- Use `Test2002.tsx` repro

## Checklist

- [x] Included code example that can be used to test this change
- [x] Updated documentation: <!-- For adding new props to native-stack
-->
- [x]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [x] Ensured that CI passes
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