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): fullscreenmodal color scheme adaptability #2211

Merged
merged 12 commits into from
Jun 26, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Jun 26, 2024

Description

This PR fixes the color scheme adaptability when using presentation: fullScreenModal.
When changing the appearance mode between light and dark the screens reacted immediately except for the fullScreenModal presentation style.

When using a UIModalPresentationFullScreen the views belonging to the presenting view controller are removed after the presentation completes. After investigating the view hierarchy for a screen with presentation: fullScreenModal it turned out that the RCTRootView is removed, thus the traitCollectionDidChange observer (see here) is not working anymore.

The solution was to add an extra observer for RNSScreenStackPresentationFullScreenModal to be able to send proper notifications when no RCTRootView is present. As the observer is working synchronously it's usage is limited for this presentation style only.

Fixes #2002.

Changes

  • added repro Test2002.tsx
  • added traitCollectionDidChange observer for fullScreenModal

Screenshots / GIFs

Before

Screen.Recording.2024-06-26.at.11.44.49.mov

After

Screen.Recording.2024-06-26.at.11.43.42.mov

Test code and steps to reproduce

  • added Test2002.tsx to test examples

Checklist

@alduzy alduzy requested review from kkafar and tboba June 26, 2024 09:42
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.

very good job!

I've asked for few changes and we're good to go.

ios/RNSModalScreen.mm Outdated Show resolved Hide resolved
ios/RNSModalScreen.mm Outdated Show resolved Hide resolved
alduzy and others added 3 commits June 26, 2024 11:59
Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
@alduzy alduzy requested a review from kkafar June 26, 2024 10:09
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 now 💪🏻 🎉

Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

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

LGTM!

@alduzy alduzy merged commit c460fb7 into main Jun 26, 2024
5 checks passed
@alduzy alduzy deleted the @alduzy/fullscreenmodal-color-scheme-fix branch June 26, 2024 12:08
alduzy added a commit that referenced this pull request Jun 28, 2024
## Description

This PR fixes the color scheme adaptability when using `presentation:
fullScreenModal`.
When changing the appearance mode between `light` and `dark` the screens
reacted immediately except for the fullScreenModal presentation style.

When using a `UIModalPresentationFullScreen` the views belonging to the
presenting view controller are removed after the presentation completes.
After investigating the view hierarchy for a screen with `presentation:
fullScreenModal` it turned out that the `RCTRootView` is removed, thus
the `traitCollectionDidChange` observer ([see
here](https://github.com/facebook/react-native/blob/d3e0430deac573fd44792e6005d5de20e9ad2797/packages/react-native/React/Base/RCTRootView.m#L362))
is not working anymore.

The solution was to add an extra observer for
`RNSScreenStackPresentationFullScreenModal` to be able to send proper
notifications when no RCTRootView is present. As the observer is working
synchronously it's usage is limited for this presentation style only.

Fixes #2002.


## Changes

- added repro `Test2002.tsx`
- added `traitCollectionDidChange` observer for fullScreenModal


## Screenshots / GIFs

### Before


https://github.com/software-mansion/react-native-screens/assets/91994767/52fa4e92-3baa-49e2-b278-7be57c4d28b3

### After

https://github.com/software-mansion/react-native-screens/assets/91994767/74c62c45-c793-4f63-81fc-d68d4000fea6


## Test code and steps to reproduce

- added `Test2002.tsx` to test examples

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
…#2211)

## Description

This PR fixes the color scheme adaptability when using `presentation:
fullScreenModal`.
When changing the appearance mode between `light` and `dark` the screens
reacted immediately except for the fullScreenModal presentation style.

When using a `UIModalPresentationFullScreen` the views belonging to the
presenting view controller are removed after the presentation completes.
After investigating the view hierarchy for a screen with `presentation:
fullScreenModal` it turned out that the `RCTRootView` is removed, thus
the `traitCollectionDidChange` observer ([see
here](https://github.com/facebook/react-native/blob/d3e0430deac573fd44792e6005d5de20e9ad2797/packages/react-native/React/Base/RCTRootView.m#L362))
is not working anymore.

The solution was to add an extra observer for
`RNSScreenStackPresentationFullScreenModal` to be able to send proper
notifications when no RCTRootView is present. As the observer is working
synchronously it's usage is limited for this presentation style only.

Fixes software-mansion#2002.


## Changes

- added repro `Test2002.tsx`
- added `traitCollectionDidChange` observer for fullScreenModal


## Screenshots / GIFs

### Before


https://github.com/software-mansion/react-native-screens/assets/91994767/52fa4e92-3baa-49e2-b278-7be57c4d28b3

### After

https://github.com/software-mansion/react-native-screens/assets/91994767/74c62c45-c793-4f63-81fc-d68d4000fea6


## Test code and steps to reproduce

- added `Test2002.tsx` to test examples

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Updated documentation: <!-- For adding new props to native-stack
-->
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
- [ ]
https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
- [ ] Ensured that CI passes

---------

Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
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.

React Native useColorScheme() hook stops working when using presentation: 'fullScreenModal'
3 participants