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: don't wrap content with BottomSheetView when enableDynamicSizing is true #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeanregisser
Copy link

@jeanregisser jeanregisser commented Sep 10, 2024

Hey,

This PR is to get the discussion started on the following problem:
We're using react-navigation-bottom-sheet with enableDynamicSizing set to true but want to use BottomSheetScrollView (or other Gorhom components: BottomSheetFlatList, BottomSheetSectionList, etc)
within the screen to get proper sizing and all features that go with it (scroll down to dismiss, etc).

Currently with the latest version of react-navigation-bottom-sheet (0.3.2), this causes sizing issues and blank renders.
I tracked it down to having both BottomSheetView (in this lib) and BottomSheetScrollView (in our own screen) nested.
They somehow conflict.

As a workaround I've patched it to remove the BottomSheetView wrapping: https://github.com/valora-inc/wallet/blob/bbd0e39dcc1420a704fa866113994e9a20c3bfa4/patches/%40th3rdwave%2Breact-navigation-bottom-sheet%2B0.3.2.patch

This works pretty well for us, but we must ensure the screens use one of the Gorhom components.

I'm not sure about the "right" solution to avoid this patch. Here are possible options:

  1. force the consumers of this lib to wrap their screens with the Gohrom component they need
  2. iterate over the children to see if one of them is a Gohrom component
    • not sure it would work for all cases, especially if the screen nests another bottom sheet that doesn't need to be a screen
  3. add a new property for devs to set when they don't want the default wrapping with BottomSheetView
  4. other?

Looking forward to finding a solution that will work for all users of this lib.

@Rag0n
Copy link

Rag0n commented Oct 24, 2024

Thanks for the PR - I'm having the same issue and wrapping everything in BottomSheetView is definitely not a right approach.

Hey @janicduplessis, what do you think?

@vipierozan99
Copy link

same issue here, maybe we could add a sheetWrapper prop and where you pass in the Gohrom component?

<BottomSheet
  screenOptions={{
    sheetWrapper: BottomSheetView
  }}
/>

@vipierozan99
Copy link

vipierozan99 commented Oct 28, 2024

Also, would it make sense to add this? as it seems like this lib assumes the old default of undefined=false

enableDynamicSizing = true, // same default as lib

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.

3 participants