-
Notifications
You must be signed in to change notification settings - Fork 80
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: ensure screens that are bottom sheets scroll correctly #5062
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,50 @@ | ||
import { BottomSheetScrollView as GorhomBottomSheetScrollView } from '@gorhom/bottom-sheet' | ||
import React, { useState } from 'react' | ||
import { LayoutChangeEvent, ScrollView, StyleProp, StyleSheet, View, ViewStyle } from 'react-native' | ||
import { LayoutChangeEvent, StyleProp, StyleSheet, View, ViewStyle } from 'react-native' | ||
import { ScrollView } from 'react-native-gesture-handler' | ||
import { useSafeAreaInsets } from 'react-native-safe-area-context' | ||
import { Spacing } from 'src/styles/styles' | ||
import variables from 'src/styles/variables' | ||
|
||
interface Props { | ||
containerStyle?: StyleProp<ViewStyle> | ||
testId?: string | ||
forwardedRef?: React.RefObject<ScrollView> | ||
isScreen?: boolean | ||
children: React.ReactNode | ||
} | ||
|
||
function BottomSheetScrollView({ forwardedRef, containerStyle, testId, children }: Props) { | ||
function BottomSheetScrollView({ | ||
forwardedRef, | ||
containerStyle, | ||
testId, | ||
isScreen, | ||
children, | ||
}: Props) { | ||
const [containerHeight, setContainerHeight] = useState(0) | ||
const [contentHeight, setContentHeight] = useState(0) | ||
|
||
const insets = useSafeAreaInsets() | ||
const scrollEnabled = contentHeight > containerHeight | ||
|
||
// Note: scrolling views inside bottom sheet screens should use the relevant | ||
// components from react-native-gesture-handler instead of directly from | ||
// react-native, otherwise they do not scroll correctly. This isScreen prop | ||
// should be set to true if the bottom sheet is registered as screen in the | ||
// Navigator. It is still handy for screens and components to share this | ||
// component for the styling and layout logic. | ||
// https://github.com/osdnk/react-native-reanimated-bottom-sheet/issues/264#issuecomment-674757545 | ||
const ScrollViewComponent = isScreen ? ScrollView : GorhomBottomSheetScrollView | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if we use the RNGH ScrollView everywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes it is the overscroll to dismiss that is not available with ScrollView. i updated the comment to be more clear about this. it's annoying that this is an issue, but i think the overscroll / bottom sheet gestures are probably the thing preventing the BottomSheetScrollView to work correctly from the navigator |
||
// use max height simulate max 90% snap point for screens. when bottom sheets | ||
// take up the whole screen, it is no longer obvious that they are a bottom | ||
// sheet / how to navigate away | ||
const maxHeight = isScreen ? variables.height * 0.9 : undefined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. annoyingly i couldn't find a better way to set the snappoints to be dynamic + set an upper bound on the height |
||
|
||
return ( | ||
<GorhomBottomSheetScrollView | ||
<ScrollViewComponent | ||
ref={forwardedRef} | ||
scrollEnabled={scrollEnabled} | ||
style={{ maxHeight }} | ||
onLayout={(event: LayoutChangeEvent) => { | ||
setContainerHeight(event.nativeEvent.layout.height) | ||
}} | ||
|
@@ -40,7 +63,7 @@ function BottomSheetScrollView({ forwardedRef, containerStyle, testId, children | |
> | ||
{children} | ||
</View> | ||
</GorhomBottomSheetScrollView> | ||
</ScrollViewComponent> | ||
) | ||
} | ||
|
||
|
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.
Maybe add a small comment to explain when/why this is needed?
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.
yes updated