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

Eli nathan/dynamic size bottom sheet with max height #1510

Closed
wants to merge 6 commits into from
Closed

Eli nathan/dynamic size bottom sheet with max height #1510

wants to merge 6 commits into from

Conversation

Eli-Nathan
Copy link

@Eli-Nathan Eli-Nathan commented Sep 8, 2023

Inspired by #1402 @ororsatti
Adding a hook to have a sheet with a max height that will dynamically change its height until reaching the limit, following this discussion: #1363

  • Added useBottomSheetMaxHeight hook
  • Call above hook from useBottomSheetDynamicSnapPoints
  • Updated the example to show the max height & scrolling functionality.
    • Example update also changes it from adding blank empty height to adding new rows of content

Motivation
People are asking for this feature for a while now, and asked me in the discussion to share the code.
Closes: (#658 , #32)

Example

Screen.Recording.2023-09-08.at.18.38.42.mov

@ororsatti
Copy link

May the odds be in your favour !

@Eli-Nathan Eli-Nathan marked this pull request as ready for review September 8, 2023 14:20
@@ -0,0 +1,39 @@
import { useWindowDimensions } from 'react-native';
import { useSafeAreaInsets } from 'react-native-safe-area-context';
Copy link
Owner

Choose a reason for hiding this comment

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

could we use internal container height instead using react-native-safe-area-context library ?

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

first, thanks @ororsatti for submitting the initial PR, i do apologies for not reviewing it on time , your contribution is appreciated.

second, @Eli-Nathan i left a comment about the usage of react-native-safe-area-context, im wondering if we could just use the internal container height value ?

@gorhom gorhom self-assigned this Sep 8, 2023
@gorhom gorhom added v4 Written in Reanimated v2 v5 labels Sep 8, 2023
@Eli-Nathan
Copy link
Author

Thanks so much!
My thinking was that bottom sheets (especially modals) can wrap the entire app including the react navigation elements like the header and bottom tabs. Using the safe area guarantees that on newer iOS devices that the bottom sheet never goes higher than the notch and covers the status bar.
Also, actually getting the parent container height would need to be done by the developer using it and passed in to this hook as the library doesn't have context of where it's rendered right? Felt like it would make user error more likely. What do you think? You'll know this code far better than myself.

@Eli-Nathan
Copy link
Author

@gorhom examples with and without safe area when modal provider wraps the whole app 😄

With safe area Without safe area

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

I have already expose the container height animatedContainerHeight from useBottomSheetInternal, which you can access with unsafe flag, since the hook is out of the bottom sheet node

const { animatedContainerHeight } = useBottomSheetInternal(true);

@Eli-Nathan
Copy link
Author

Eli-Nathan commented Sep 8, 2023

It's telling me it's not available 😞

Screenshot 2023-09-08 at 19 54 38

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

hmmm ,, let me think of a different approach .. my main concern is now we are adding another hard dependency to the project which is something i try to avoid

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

i was thinking about this for a while now, what if we moved the while dynamic sizing internally , where users do not need to do anything 🧐 , except using a view from the library ( scrollable or just static view )

@Eli-Nathan
Copy link
Author

Edit: Ah wait it works without the boolean...
Although technically we wouldn't be using it from within the bottom sheet so the internal context throws an error

@Eli-Nathan
Copy link
Author

Eli-Nathan commented Sep 8, 2023

where users do not need to do anything

Hmm, would that still allow them to specify a maxHeight though?

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

we could introduce a prop for max snap point to address that issue, so users who wants dynamic size they will need to provide two props:

<BottomSheet enableDynamicSizing={true} maxSnapPoint={500 | "100%"} />

with this, we will remove the dynamic hook and remove handling layout complexity

@Eli-Nathan
Copy link
Author

So we'd be applying these styles to <BottomSheetContainer /> within the main bottom sheet component based on the presence of a enableDynamicSizing prop?

I've had a look at that component and it's very complex.

What are your thoughts on doing applying this as a solution for V4 and the enableDynamicSizing solution for V5 since it seems like it'd be a much bigger refactor?

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

So we'd be applying these styles to within the main bottom sheet component based on the presence of a enableDynamicSizing prop?

no, i would apply it to the content container and scrollables

What are your thoughts on doing applying this as a solution for V4 and the enableDynamicSizing solution for V5 since it seems like it'd be a much bigger refactor?

mm i think the code should be sharable with v5 if we do it correctly for v4. im going to draft something as a poc and get back to you

@Eli-Nathan
Copy link
Author

Sounds good! Thanks again!

@ororsatti
Copy link

i was thinking about this for a while now, what if we moved the while dynamic sizing internally , where users do not need to do anything 🧐 , except using a view from the library ( scrollable or just static view )

First of all, I'm so glad to hear from you, I thought this lib is going to dei down slowly,
Second, It seems like a big project and I'll be more than happy to help move it, and v5.
I'd be happy to contact you & @Eli-Nathan and maybe get a quick rundown on your lib, and start helping you with it.

@Eli-Nathan
Copy link
Author

@ororsatti Count me in, this has been hugely valuable to me in multiple projects so happy to help where I can

@gorhom
Copy link
Owner

gorhom commented Sep 8, 2023

@Eli-Nathan @ororsatti managed to get dynamic sizing by prop working with View with the approach i suggested , going to apply it on scrollables and i'll prepare a draft PR by tomorrow

@gorhom
Copy link
Owner

gorhom commented Sep 10, 2023

closing this PR in favour of #1513, which it has been merged. Thanks @Eli-Nathan

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.

3 participants