Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use user-provided onScroll #22

Merged
merged 4 commits into from
Sep 27, 2023
Merged

feat: use user-provided onScroll #22

merged 4 commits into from
Sep 27, 2023

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
3 checks passed
@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