Skip to content

Conversation

@roryabraham
Copy link

@roryabraham roryabraham commented Sep 15, 2021

Hi @naqvitalha 👋

Thanks for all your hard work on this repo so far! I'm exploring the possibility of integrating this repo into Expensify, and one of our requirements is an infinite bidirectional scrolling list, and I hope that we can work together to bring this desperately-needed functionality to the React Native community.

So this pull request fixes the following issues:

  1. Bidirectional Pagination List #613
  2. Resizing the window can break scrolling #648
  3. A third issue on Android where onEndReached would not be called unless an onEndReachedThreshold is provided.

iOS

IOSBidirectionalScroll.mov

Web

WebBidirectionalScroll.mov

Android

AndroidBidirectionalScroll.mov

Copy link

@kidroca kidroca left a comment

Choose a reason for hiding this comment

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

Added some notes

import BaseScrollComponent, { ScrollComponentProps } from "../../../core/scrollcomponent/BaseScrollComponent";
import BaseScrollView, { ScrollEvent } from "../../../core/scrollcomponent/BaseScrollView";
import ScrollViewer from "./ScrollViewer";
import debounce = require("lodash.debounce");
Copy link

Choose a reason for hiding this comment

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

Suggested change
import debounce = require("lodash.debounce");
import debounce from "lodash.debounce";

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, something funky is going on here and I think that the dynamic require might be necessary. It was like this elsewhere in the library too, but IDK why 🤷

Comment on lines +462 to +464
const onStartReachedCalled = this._onStartReachedCalled;
if (newProps.dataProvider.getSize() > this.props.dataProvider.getSize()) {
this._onEndReachedCalled = false;
this._setOnEdgeReachedCalled(false);
Copy link

Choose a reason for hiding this comment

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

We're caching the current value of onStartReached but it could change (to false) if we enter the next if block
Then the following expression can still pass

if (viewabilityTracker && onStartReachedCalled) {

Can't we use this._onStartReachedCalled there?

Having to cache the value and not use the latest seems a bit fishy

@roryabraham roryabraham changed the title Add support for onStartReached Add support for bidirectional infinite scrolling Oct 8, 2021
@roryabraham roryabraham marked this pull request as ready for review October 8, 2021 22:00
@roryabraham
Copy link
Author

roryabraham commented Oct 8, 2021

@naqvitalha I've taken this out of draft and would love a review when you have some time 🙏

I'm happy to adjust the PR or answer any questions you might have. Thanks 😄

Example implementation (modified from the example created in this Youtube video series): https://github.com/roryabraham/RecyclerListViewExample

@roryabraham
Copy link
Author

Disclaimer: I tested in react-native-web but not pure React.JS

@eseQ
Copy link

eseQ commented Feb 3, 2022

What about merge this PR?

@zhangwen9229
Copy link

Why this pr was not continue?
It is a cool feature.

@thix-tech
Copy link

thix-tech commented Sep 26, 2022

I also need this feature. Hope to merge soon.

@roryabraham
Copy link
Author

I don't have plans of continuing this implementation anytime soon because we ended up implementing bidirectional pagination directly in React Native's VirtualizedList.

For anyone who wants to continue with this PR, I got stuck with a bug where content would sometimes infinitely load until you reached the start of the list because maintainVisibleContentPosition wasn't working. This same bug occurred in React Native's VirtualizedList, and we fixed it here.

The problem is explained here

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.

5 participants