Spring animation, over scroll and android bug fix #39
Conversation
There was a problem hiding this comment.
Hey @dcoulter45,
This is great work, thanks very much! 🎸
I'd really love to see this PR merged to roll out a spring configuration, but making sure there are no regressions and it's fully backwards compatible. The optional overscroll is a very nice touch, kudos!
To facilitate the merge, I've created a draft PR based on yours that you can follow as a reference for some of the comments I've left.
Also, if you wouldn't mind, could you also document the new props in the implicit props table?
Happy to help in case you have any more doubts/questions, just lemme know 🙂
| @@ -333,7 +362,9 @@ export class ScrollBottomSheet<T extends any> extends Component<Props<T>> { | |||
| const isAnimationInterrupted = and( | |||
| or( | |||
| eq(handleGestureState, GestureState.BEGAN), | |||
| eq(drawerGestureState, GestureState.BEGAN) | |||
| eq(handleGestureState, GestureState.ACTIVE), | |||
| eq(drawerGestureState, GestureState.BEGAN), | |||
There was a problem hiding this comment.
I've been testing in depth your branch. I found out those two new lines introduced more problems than solutions. I am aware that sometimes the BS gets stuck in the middle (with 2 snap points) and only with spring animation type, but it's rare and it settles back quickly. Conversely I discovered new and more severe regressions, so better to remove them for now.
There was a problem hiding this comment.
What regressions did you notice? For me the BS was getting stuck quite often and didn't settle back on it's own.
There was a problem hiding this comment.
Ok, I'll try to reproduce it again.
Could you please tell me:
- Platform: Android/iOS
- BS configuration,
snapPoints
Or otherwise, provide me with a snack/example where you saw that happening quite often?
There was a problem hiding this comment.
I saw a regression of the BS getting stuck on the bottom position quite often, where you couldn't pull it up for a while (you could still scroll though). Then it suddenly jumped to middle or up position.
Also, I experienced more abruptness when quickly interacting with it, or interrupting an animation by a gesture, like there was a degradation on the smoothness.
Let's assess it again, worst case scenario we can add those 2 lines under a condition of the animation being spring only, so timing doesn't get affected.
Btw, I have in the roadmap to refactor the logic to leverage reanimated 2, once it gets more stable. That will most likely reduce the complexity of the code and will help external contributions to be easier to deal with.
There was a problem hiding this comment.
iOS, RN 0.61. To replicate you need to pull the drawer upwards and pull it down before it reaches the top snap point.
I think I know the issues you mean now. They seem to happen on android? Since adding this only fixes an issue on iOS, it could be added under a condition of both spring animation & iOS?
| @@ -715,15 +756,19 @@ export class ScrollBottomSheet<T extends any> extends Component<Props<T>> { | |||
| const { method, args } = imperativeScrollOptions[ | |||
| this.props.componentType | |||
| ]; | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
Interesting, did you get an error thrown or something? How did you detect this issue visually?
There was a problem hiding this comment.
Yeah, was throwing this exception on android.
There was a problem hiding this comment.
Got it, was that maybe happening on RN 0.62 or 0.63?
There was a problem hiding this comment.
Happening on RN 0.61. Have a couple of BS's rendered at once, split between a few tabs. But not sure if it's related.
No problem, made those last changes. Hopefully it all looks good. |
Spring animation is based off @tankers746 pull request, so thank you to him for that.