-
-
Notifications
You must be signed in to change notification settings - Fork 734
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
feat: added dynamic sizing #1513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing! Nice work!
Does this remove the ability for users to set initial snap points and have CONTENT_HEIGHT as a specific snap point?
@@ -48,7 +48,7 @@ export interface BottomSheetProps | |||
* snapPoints={['%100']} | |||
* @type Array<string | number> | |||
*/ | |||
snapPoints: Array<string | number> | SharedValue<Array<string | number>>; | |||
snapPoints?: Array<string | number> | SharedValue<Array<string | number>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything we can do with the type annotation to tell users that this is only not required when the enableDynamicSizing prop is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm not sure how could we achieve that, but if customers did not provide snap points and dynamic sizing prop, it will throw an exception https://github.com/gorhom/react-native-bottom-sheet/pull/1513/files#diff-620b7ea703632ea948b10134aaea17e0efd09781b64f587ecab4395812f9a3ccR26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in the JSDoc comment above I was thinking. Should show in most IDEs when you hover over the prop or while you're typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eli-Nathan i have added an extra description in the latest commit
users could provide their own snap points, and when the layout is calculated, the logic will insert the content size in the snap points list and sort it accordingly |
Tested, looks awesome ! |
When this is expected to be released? |
@ororsatti this has been released 10 minutes ago |
@Eli-Nathan @ororsatti @younes0 please have a look at the docs before using this prop
|
Scratch that, had to dump my node_modules and reinstall |
It's working perfectly here! Great work on this! A reminder if you are having trouble with the dynamic sizing, don't use Don't use this:
Use something like this:
|
enableDynamicSizing does not work for me, my BottomSheet is stuck at 0px height |
@gabrieldonadel Can you try passing a sharedValue for the contentHeight prop?
|
It works with |
@rodolphoasb how do you handle safe area in your case ? |
https://github.com/th3rdwave/react-native-safe-area-context#usesafeareainsets |
@todorone thanks for the documentation which I'm already aware of but this is not helpful. |
working example |
@fendermany I believe this example is only working when including
|
Any example with both a fixed view and a scrollable content inside? <BottomSheetModal>
<View>
<Text>Some fixed view</Text>
<BottomSheetScrollView>
{/* some scrollable list */}
</BottomSheetScrollView>
</View>
</BottomSheetModal> With Record_2023-11-03-14-16-19.mp4In the video you can see as I switch the tabs, the content is still scrollable even when the height for the new content is well below the <BottomSheetModal
ref={bottomSheetModalRef}
enableDynamicSizing
maxDynamicContentSize={700}
handleIndicatorStyle={{ display: 'none' }}
backdropComponent={CustomBackdrop}
enableContentPanningGesture={false}>
<View style={{ flex: 1 }}>
<BottomSheetView>
</BottomSheetView>
<BottomSheetScrollView>
{/* some scrollable list */}
</BottomSheetScrollView>
</View>
</BottomSheetModal> However using Record_2023-11-03-15-04-26.mp4As you can see here the new content is being rendered fully without being scrolled. const initialSnapPoints = useMemo(() => ['CONTENT_HEIGHT'], []);
const {
animatedHandleHeight,
animatedSnapPoints,
animatedContentHeight,
handleContentLayout,
} = useBottomSheetDynamicSnapPoints(initialSnapPoints);
const useBottomSheetMaxHeight = (maxHeight = '85%'): number => {
const { height: deviceHeight } = useWindowDimensions();
const maxHeightDecimal = parseFloat(`${maxHeight}`) / 100.0;
const { top: topSafeAreaInset } = useSafeAreaInsets();
return (deviceHeight - topSafeAreaInset) * maxHeightDecimal;
};
const maxHeightStyle = useBottomSheetMaxHeight('60%');
...
...
<BottomSheetModal
ref={bottomSheetModalRef}
snapPoints={animatedSnapPoints as any}
handleHeight={animatedHandleHeight}
contentHeight={animatedContentHeight}
handleIndicatorStyle={{ display: 'none' }}
backdropComponent={CustomBackdrop}
enableContentPanningGesture={false}>
<View style={{ flex: 1 }} onLayout={handleContentLayout}>
<BottomSheetView>
</BottomSheetView>
<BottomSheetScrollView
</BottomSheetScrollView>
</View>
</BottomSheetModal> |
@badalsaibo yeah I'm still having to use my max height solution alongside this too although I'm able to use it with the new const MyBottomSheetModal: FunctionComponent<BottomSheetModalProps> = ({ bottomSheetModalRef, maxHeight }) => {
const calculatedMaxHeight = useBottomSheetMaxHeight(maxHeight)
return (
<BottomSheetModal
ref={bottomSheetModalRef}
enableDynamicSizing
maxDynamicContentSize={calculatedMaxHeight}
// ... more props
>
<BottomSheetScrollView style={{ maxHeight: calculatedMaxHeight }}>{children}</BottomSheetScrollView>
</BottomSheetModal>
... useBottomSheetMaHeight hook: /*
Designed to take the specified max height (or default 85%)
and take away top safe area inset
*/
import { useWindowDimensions } from "react-native"
import { useSafeAreaInsets } from "react-native-safe-area-context"
import type { UseBottomSheetMaxHeight } from "./types"
type MaxHeight = `${string}%` | number
const getPercentageMaxHeight = (
maxHeight: `${string}%`,
safeMaxheightValue: number
) => {
return safeMaxheightValue * (parseFloat(maxHeight) / 100)
}
const getNumberMaxHeight = (maxHeight: number, safeMaxheightValue: number) => {
if (maxHeight > safeMaxheightValue) {
return safeMaxheightValue
}
return maxHeight
}
export const useBottomSheetMaxHeight: UseBottomSheetMaxHeight = (
maxHeight?: MaxHeight
) => {
const { height: deviceHeight } = useWindowDimensions()
const { top: topSafeAreaInset } = useSafeAreaInsets()
if (!maxHeight) {
return undefined
}
const safeDeviceHeight = deviceHeight - topSafeAreaInset
const maxHeightIsPercentage = typeof maxHeight === "string"
const maxHeightNumber = maxHeightIsPercentage
? getPercentageMaxHeight(maxHeight as `${string}%`, safeDeviceHeight)
: getNumberMaxHeight(maxHeight as number, safeDeviceHeight)
return maxHeightNumber
} This allows you to pass a specific number as he max height, or a percentage Note that I'm using the Hope that helps |
I don't know man. For some reason it behaves the way as in the first video even with the new I have no idea what's going on but I'll be saving a copy of |
//#region callbacks | ||
const handleContentSizeChange = useStableCallback( | ||
(contentWidth: number, contentHeight: number) => { | ||
animatedContentHeight.value = contentHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean that I can't have additional content inside my bottom sheet if I'm using a scrollable component? The height of the sheet will be set to the height of the scrollable component instead of using the height of all the content in the sheet..
When will this be added to v5? I have migrated to a high v4 but would like to use |
I'm not able to get this working with React Navigation Integration, is there any limits to |
@audn my experience was that when using a nested navigator, a fixed height for the sheet was necessary. |
I don't know if someone else still have the same issue, but here is my quick implementation for the bottom sheet with dynamic height
|
I don't really see what you're doing differently? Also I don't see what styles you're applying, if that's any important. |
Closes #658, closes #32, closes #1363
Motivation
This PR is a replacement for #1510 by @Eli-Nathan & #1402 by @ororsatti, where the implementation been shifted into the core library allowing the removal of the
useBottomSheetDynamicSnapPoints
hook.Users will be able to enable dynamic sizing by only providing a boolean prop into the bottom sheet component
enableDynamicSizing
. And enable setting a limit for content height by providing a max value withmaxDynamicContentSize
.Testing