feat: use user-provided onScroll

ythomop commented 10 months ago

Description

Runs the user-provided onScroll function after react-native-header's scrollHandler

Motivation and Context

Issue

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[X] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[X] I have followed the guidelines in the README.md file.
[X] I have updated the documentation as necessary.
[X] My changes generate no new warnings.

e-younan commented 10 months ago

Thanks for opening this PR! I will review this and add an example usage to the example application to ensure it works for all the scroll containers.

ythomop commented 10 months ago

Sorry, I made this PR in haste, without reason, without cross-checking this implementation with the one that I actually use in my project. Your implementation is ok, although I would prefer to either have the original ScrollView API with onScroll actually working as expected or remove onScroll altogether from the available props and only keep onScrollWorklet. However, it's not that big of a problem, so if you're ok, I'm ok with this PR.

The actual code that I use and works is the following:

  const scrollHandler = useAnimatedScrollHandler((event) => {
    scrollY.value = event.contentOffset.y;
    // user-provided onScroll of the type "(evt: NativeScrollEvent) => void;"
    if (onScroll) runOnJS(onScroll)(event);
  });

e-younan commented 10 months ago

Sorry, I made this PR in haste, without reason, without cross-checking this implementation with the one that I actually use in my project. Your implementation is ok, although I would prefer to either have the original ScrollView API with onScroll actually working as expected or remove onScroll altogether from the available props and only keep onScrollWorklet. However, it's not that big of a problem, so if you're ok, I'm ok with this PR.

The actual code that I use and works is the following:
  const scrollHandler = useAnimatedScrollHandler((event) => {
    scrollY.value = event.contentOffset.y;
    // user-provided onScroll of the type "(evt: NativeScrollEvent) => void;"
    if (onScroll) runOnJS(onScroll)(event);
  });

The code here is executing the onScroll method on the JS thread - I would prefer to stick with worklets and allow the developer to do a runOnJs call explicitly if they need to do so.

You made a good point earlier and I will adjust the implementation to remove the onScroll property altogether.

ythomop commented 10 months ago

I think it's good to go! 😀

e-younan commented 10 months ago

@ythomop It's merged and released under version 0.11.0 now. Thanks for the PR!

codeherence / react-native-header