fix: ensure screens that are bottom sheets scroll correctly #5062
Conversation
| // 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.
annoyingly i couldn't find a better way to set the snappoints to be dynamic + set an upper bound on the height
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5062 +/- ##
=======================================
Coverage 85.53% 85.53%
=======================================
Files 721 721
Lines 29413 29422 +9
Branches 5083 5085 +2
=======================================
+ Hits 25159 25167 +8
- Misses 4021 4022 +1
Partials 233 233
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| // 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.
What happens if we use the RNGH ScrollView everywhere?
Does it break any gesture? Maybe the overscroll to dismiss?
There was a problem hiding this comment.
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
|
|
||
| interface Props { | ||
| containerStyle?: StyleProp<ViewStyle> | ||
| testId?: string | ||
| forwardedRef?: React.RefObject<ScrollView> | ||
| isScreen?: boolean |
There was a problem hiding this comment.
Maybe add a small comment to explain when/why this is needed?
…nc#5062) ### Description Context https://valora-app.slack.com/archives/C025V1D6F3J/p1709855852378389 The bottom sheet screens were not scrolling correctly, this was probably a regression but unnoticed because most of these screens do not require scrolling. Root cause is a known issue that is documented in a comment in the Navigator - we need to use the react-native-gesture-handler components to scroll inside bottom sheet screens. This PR adds a prop to the BottomSheetScrollView component to use the correct scrollview component. ### Test plan Scrolling content: https://github.com/valora-inc/wallet/assets/20150449/349c2628-017c-46b1-af88-06d20bcb19ce Short content: https://github.com/valora-inc/wallet/assets/20150449/5929f8f9-08f1-495f-8131-211cd055fbdb Tested also on the WC flow. ### Related issues - n/a ### Backwards compatibility Y ### Network scalability Y
Description
Context https://valora-app.slack.com/archives/C025V1D6F3J/p1709855852378389
The bottom sheet screens were not scrolling correctly, this was probably a regression but unnoticed because most of these screens do not require scrolling. Root cause is a known issue that is documented in a comment in the Navigator - we need to use the react-native-gesture-handler components to scroll inside bottom sheet screens.
This PR adds a prop to the BottomSheetScrollView component to use the correct scrollview component.
Test plan
Scrolling content:
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2024-03-08.at.11.07.42.mp4
Short content:
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2024-03-08.at.11.08.02.mp4
Tested also on the WC flow.
Related issues
Backwards compatibility
Y
Network scalability
Y