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(bottom-sheet-modal): behind native modal view #1309

Merged

Conversation

magrinj
Copy link
Contributor

@magrinj magrinj commented Mar 2, 2023

Fix issue #832

Motivation

Using react-navigation or native Modal component with BottomSheetModal component will show them behind the native views.

As suggested in react-native-portal library, we should wrap the content inside FullWindowOverlay to get the view above all the native views.

https://github.com/gorhom/react-native-portal#react-native-screens-integration

I had initially create this PR #1086 to fix the issue, but this one fix the concern about adding react-native-screens as a required dependency.

react-native-screens is now added but as an optional peer dependency.
If the module is installed the FullWindowOverlay will be used on iOS, otherwise it will work as before.

@joseortiz9
Copy link

@gorhom hope you can check this PR, nice work
UP +1

@batuhansahan
Copy link

this does not work with react navigation ios modals(presentation: 'modal'). bottom-sheet bottom insets jumping up.

@magrinj
Copy link
Contributor Author

magrinj commented Mar 17, 2023

@batuhansahan Not sure to understand what is actually not working on your side, can you provide a sample ?

@batuhansahan
Copy link

batuhansahan commented Mar 17, 2023

bottom sheet looks like this. I properly set up <BottomSheetModalProvider> And BottomSheetModal. bottominset is already 0,
the goal of using your pr because hoping to fix that where swiping down to dismiss a bottom sheet was also closing the underlying ios-modal behind it, PR is fixing the swipe conflict WELL DONE!. but bottom sheet is not in correct position.
simulator_screenshot_4FDA2970-284F-4086-A8C9-28EBAF684CE8

@magrinj
Copy link
Contributor Author

magrinj commented Mar 17, 2023

Hmm strange, maybe your BottomSheetModalProvider is not correctly placed.
On my side I'm using this library https://github.com/th3rdwave/react-navigation-bottom-sheet
It allows to use bottom sheet with the same api as react-navigation and it's working really well on my side with presentation modal using native stack from react-navigation.

@batuhansahan
Copy link

batuhansahan commented Mar 18, 2023

i tried very way

-provider
--navigation container

and reverse.

@thecoorum
Copy link

It's also not working for me with the following screen options

animation: 'none',
presentation: 'transparentModal'

It's always rendered behind the stack screen

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@magrinj
Copy link
Contributor Author

magrinj commented May 14, 2023

@gorhom Any news on this ?

@thecoorum
Copy link

It is working for me with the BottomSheetModalProvider around NavigationContainer when my screens have the following options

animation: 'none',
presentation: 'transparentModal'

The only thing is that screens are implemented with regular BottomSheet, but some nested modals with BottomSheetModal. I think if implementing both with modals there is some portaling issue

@erickreutz
Copy link

Would love to see this merged!

@thecoorum
Copy link

I noticed that this implementation conflicts with libraries like react-native-ios-context-menu. If you are rendering dropdown menu in such modal it will be just invisible

@magrinj
Copy link
Contributor Author

magrinj commented Jun 2, 2023

@thecoorum Actually it’s not invisible but behind the bottom sheet because of the FullWindowOverlay, maybe I can add a boolean prop to enable/disable the use of the FullWindowOverlay with a default value of true.
But when disabled the problem of the bottom sheet behind modals will appear again.
It really depends on how your screens are implemented and looks like, the main goal of this fix is to solve the issue when using @react-navigation/native-stack modals, bottom sheet appear behind them.

@gorhom
Copy link
Owner

gorhom commented Jun 4, 2023

@magrinj thanks for submitting this CR, i was thinking about exposing a prop in the BottomSheetModal component that allows you to warp the bottom sheet with your own container, in your case is the react-native-screens::FullWindowOverlay ?

by doing this, the library still take no dependency ( other than GH and REA ) and you get to customising the container as you please .

@magrinj
Copy link
Contributor Author

magrinj commented Jun 5, 2023

@gorhom Ok, I was going this way at fist so it's automatic. But I'll change my PR, and add a prop called bottomSheetWrapper

@magrinj magrinj force-pushed the fix/bottom-sheed-modal-behind-native-view branch from ecde05d to fa3fd44 Compare June 5, 2023 14:17
@magrinj
Copy link
Contributor Author

magrinj commented Jun 5, 2023

@gorhom Done ! :)

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/bottomSheetModal/BottomSheetModal.tsx Outdated Show resolved Hide resolved
@magrinj
Copy link
Contributor Author

magrinj commented Jun 6, 2023

@gorhom I just edit regarding the requested changes

@gorhom gorhom added v4 Written in Reanimated v2 v5 labels Jun 23, 2023
@gorhom gorhom merged commit 1ecad69 into gorhom:master Jun 23, 2023
@gorhom
Copy link
Owner

gorhom commented Jun 23, 2023

thanks @magrinj for submitting this PR , it should be released on v4.4.8 and v5

gorhom pushed a commit that referenced this pull request Jun 25, 2023
…)(by @magrinj)

* fix: bottom sheet modal appear behind native views

* feat: add bottomSheetWrapper prop to specify a custom wrapper component

* fix: edit regardless of the comments
@peterpme
Copy link

Hey @gorhom just a lil nudge to publish 4.4.8 if possible 😄 thank you!

@MarlonAEC
Copy link

Hey @gorhom sorry for pushing but do you have any updates on release 4.4.8? I developing a scenario in which I would like to leverage this PR it will be awesome! Thanks in advance!

@iraybi
Copy link

iraybi commented Sep 3, 2023

Anyone found any workaround to show BottomSheetModal on top of native Modal?

@thecoorum
Copy link

I think this is something that may help here
https://github.com/intergalacticspacehighway/react-native-z-view

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 Written in Reanimated v2 v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants