search

colorfy-software / react-native-modalfy

🥞 Modal citizen of React Native.
https://colorfy-software.gitbook.io/react-native-modalfy
MIT License
1.16k stars 48 forks source link

fix: displaying the modal in a screen with presentation: modal on iOS #149

Closed fobos531 closed 5 months ago

fobos531 commented 5 months ago

Fixes https://github.com/colorfy-software/react-native-modalfy/issues/125

This is actually a super simple solution, all that is needed is to just wrap the ModalStack in a FullWindowOverlay provided by react-native-screens.

This now works on iOS both with presentation: modal and regular screens.

The behavior is unchanged on Android since it is unaffected by this problem.

@CharlesMangwa I would very much appreciate if you could take a look at this and review it.

Also, there are a couple of things that could be done to modernise this library, like updating gesture handler to v2, as well as migrating to Reanimated. Would you be open to reviewing a PR doing the migration to reanimated 3, like https://github.com/colorfy-software/react-native-modalfy/pull/68?

CharlesMangwa commented 5 months ago

hey @fobos531!

thanks a lot for the pr, that's a nice fix indeed 👍

i would be more than happy to welcome a pr that'd bring rngh v2 reanimated v3!

unfortunately, i do not have much time to work on it myself lately but if you're up for it, just let me know how i could be of any assistance.

fobos531 commented 3 months ago

@CharlesMangwa Would it be possible to get this published?

fobos531 commented 3 months ago

Hello @CharlesMangwa, an important thing to note. Maybe it might be a good idea to revert this PR, because I found a use case where this implementation doesn't play well. For example, if you have a modal with a video inside (e.g. expo-video) and you use the "native controls" and want to go full screen, or change the playback speed, it will not work, because the FullWindowOverlay is placed at the root of the view hierarchy, so it seems like the native views are then not displayed properly. Therefore, it might be a good idea to revert this PR until a better solution is found.

CharlesMangwa commented 3 months ago

hey @fobos531! thanks for looking into this and letting me know, much appreciated! reverting this pr back for now.