scrollView losing position + please make code more contributor-friendly

[vonovak]

Bug

Hello, firstly, thanks for a nice lib. I'd like to report a bug and ask for the recommended solution. The bug can be reproduced using the example app, on the v4 branch (at commit 2599f6c).

I'm working on a bigger feature that customizes the interaction between the PanGestureHandler that lives "above" the scrollView and the scrollView itself. This is an issue that I found along the way:

Steps To Reproduce

scroll the scrollview to about 50% of the content, then drag the sheet down using the handle, and then slightly scroll the content.

The scrollView loses its position (offset)

I expect the scrollView to not lose its scrolling offset.

Simulator.Screen.Recording.-.iPhone.11.-.2021-05-27.at.13.09.32.mp4

The reason the bug happens are the imperative scrollTo calls in

react-native-bottom-sheet/src/hooks/useScrollableInternal.ts

Line 40 in 2599f6c

const handleScrollEvent = useAnimatedScrollHandler({

Now, I am fine with implementing a fix for this and contributing it to the repo, but wanted to ask you opinion on how you think this should be fixed.

Currently, both the scrollview (SV) and the pan gesture handler (PGH) are responders to the dragging.

Comments / questions

1. would you mind sharing the purpose of scrollableContentOffsetY and _rootScrollableContentOffsetY in

https://github.com/gorhom/react-native-bottom-sheet/blob/v4/src/hooks/useScrollableInternal.ts

?

2. question on current impl:

Using waitFor so that only either the PGH or SV (but not both) probably won't work - consider this:

0. enableContentPanningGesture=true
1. bottom sheet has 3 snap points (bottom, middle, up)
2. bottom sheet is at the middle snap point and SV is scrolled to the middle of its content
   3a) user starts to drag the SV up -> we want the whole bottom sheet to go up and SV stays in position (SV waited for PGH to become responder)
   3b) user starts to drag the SV down -> we want the SV to scroll to the beginning and the bottom sheet to stay in position (so SV did not wait for PGH) ---> this is in discrepancy with (3a) which is why I don't think waitFor works for this.

Is that why you chose the approach that is implemented?

3. comment on code readability

As I digged through the code base I noticed it's a little hard to read for newcomers / external contributors - there are a lot of conditions that are unclear. eg. thing like

react-native-bottom-sheet/src/hooks/useInteractivePanGestureHandler.ts

Line 231 in 2599f6c

if (

react-native-bottom-sheet/src/hooks/useInteractivePanGestureHandler.ts

Line 121 in 2599f6c

const negativeScrollableContentOffset =

it would be really nice if you could break those down into named constants so that those conditions have some semantic meaning, break down long functions where it's hard to know much about what is going on and why 😅, and give those functions names.
Even names like useInteractivePanGestureHandler do not provide much detail on what the hook does - I mean, isn't a gesture handler interactive by definition? A more descriptive name or comments would tell me more about what the use of this hook is 🙂

Please note I am by no means trying to criticize and I do very much value your work on this, it's just a little hard to read code that is not optimized for reading.

Thank you! :)

Environment info

libraries are used as in the example app @2599f6cf46af0f95812e34670de5a7cae5d44fd9
edited:

Library	Version
@gorhom/bottom-sheet	4.0.0-alpha.4
react-native	0.64.1
react-native-reanimated	2.1.0
react-native-gesture-handler	1.10.3

[github-actions]

@vonovak: hello! 👋

This issue is being automatically closed because it does not follow the issue template.

[RohovDmytro]

Epic detailed well-composed issue info!

[gorhom]

thanks @vonovak for this well detailed issue, this deserve a well proper reply.

I will get back to you tomorrow 👍

[gorhom]

hi @vonovak, for the issue with scrollable losing its position when dragged by handle , it is a known issue and i will make sure to fix it with v4.

would you mind sharing the purpose of scrollableContentOffsetY and _rootScrollableContentOffsetY in useScrollableInternal.ts

Each scrollable will have its own scrollableContentOffsetY and it will make sure to sync it with the root one rootScrollableContentOffsetY, all logic in BottomSheet will use rootScrollableContentOffsetY.

This was added to allow multiple scrollables in one BottomSheet, for example: the integration with React Navigation, where we have one BottomSheet with multiple screens with their own scrollable.

question on current impl: Using waitFor so that only either the PGH or SV (but not both) probably won't work - consider this:

In current implementation, i have ditch the usage of waitFor because it was blocking the usage of any touchable in BottomSheet content. So i rely on the new scroll method from react-native-reanimated to lock the scrollable position.

The solution for this issue would be to utilise animated state variables to lock and unlock scrolling.

comment on code readability
As I digged through the code base I noticed it's a little hard to read for newcomers / external contributors - there are a lot of conditions that are unclear.
it would be really nice if you could break those down into named constants so that those conditions have some semantic meaning, break down long functions where it's hard to know much about what is going on and why 😅, and give those functions names.
Even names like useInteractivePanGestureHandler do not provide much detail on what the hook does - I mean, isn't a gesture handler interactive by definition? A more descriptive name or comments would tell me more about what the use of this hook is 🙂

Please note I am by no means trying to criticize and I do very much value your work on this, it's just a little hard to read code that is not optimized for reading.

First, thanks for highlighting this, i know this code got too much complicated especially useInteractivePanGestureHandler.ts ( it became like a red zone for any contribution ), my goal is to write in-code description for each logic in the library, also the breakdown of useInteractivePanGestureHandler into smaller chunks that could be easily read.

This is something i already planned for v4 #427 , but it is good to be reminded about its important as you did 👏

Please feel free to highlight ambiguity in the code-base and i would make sure to add more descriptive in-code docs.

Thanks :)

[gorhom]

resolved here #427 (comment)