Skip to content

Conversation

@ythomop
Copy link
Contributor

@ythomop ythomop commented Sep 24, 2023

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)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

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

@e-younan
Copy link
Member

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.

Copy link
Member

@e-younan e-younan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ythomop If you have a moment, I implemented the changes I cited earlier - feel free to review the changes. I have tested the implementation and it works.

@ythomop
Copy link
Contributor Author

ythomop commented Sep 27, 2023

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
Copy link
Member

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
Copy link
Contributor Author

ythomop commented Sep 27, 2023

I think it's good to go! 😀

@e-younan e-younan merged commit f8718b4 into codeherence:main Sep 27, 2023
@e-younan
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants