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

Allow for custom ScrollStateDetector implementation #757

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

chggr
Copy link
Contributor

@chggr chggr commented Jan 21, 2021

Summary

Support for a default ScrollStateDetector n HorizontalScrollSpec and VerticalScrollSpec was added
in a previous PR. This PR allows users to provide their own custom ScrollStateDetector implementation.
It breaks the existing ScrollStateDetector class into two: It extracts an interface that users can potentially
implement with their own functionality and also creates an additional DefaultScrollStateDetector that now
contains the default implementation to be used when no custom implementation has been specified.

Changelog

Allow for custom ScrollStateDetector implementations in HorizontalScrollSpec and VerticalScrollSpec.

Test Plan

This change has been tested via unit tests

This change breaks the existing ScrollStateDetector implementation
into two. It extracts an interface that users can potentially
implement to provide their own ScrollStateDetector. It also creates
an additional DefaultScrollStateDetector class that now contains the
default ScrollStateDetector implementation.
This change adds onInterceptTouchEvent support to ScrollStateDetector
and the relevant hooks in LithoScrollView and HorizontalScrollSpec.
The implementation in the DefaultScrollStateDetector will be a no-op,
because in that class all detection is done through onTouchEvent.
Finally it also adds documentation in the ScrollStateDetector
interface.
Copy link
Contributor

@colriot colriot left a comment

Choose a reason for hiding this comment

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

Hi @chggr !
I've left a couple of comments below, but @arpitratan would probably add more after checking the actual functionally.

Can you also add a line to CHANGELOG.md about the new/changed functionality to keep everyone updated, please?

This change addds @nullable for ScrollStateDetector in both the
HorizontalScrollSpec and VerticalScrollSpec. It also changes the
method names in DefaultScrollStateDetectorTest to conform to the
Litho standard.
@chggr
Copy link
Contributor Author

chggr commented Feb 11, 2021

Thanks @colriot for the review, great feedback! I have made all required changes, please let me know if there is anything else outstanding. Many thanks again and have a great day :-)

@chggr chggr requested a review from colriot February 25, 2021 14:29
@chggr
Copy link
Contributor Author

chggr commented Mar 31, 2021

Hi @arpitratan, @colriot, I hope you are well!

Please let me know if you have any feedback on this PR.

@facebook-github-bot
Copy link
Contributor

@colriot has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

3 participants