Skip to content

WIP: Web support #16

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

WIP: Web support #16

wants to merge 10 commits into from

Conversation

vishalnarkhede
Copy link
Contributor

@vishalnarkhede vishalnarkhede commented May 24, 2021

@vishalnarkhede vishalnarkhede changed the base branch from vishal/web-support to main May 24, 2021 14:12
@vishalnarkhede vishalnarkhede changed the title Vishal/web support onrefresh WIP: Web support May 24, 2021
@vishalnarkhede vishalnarkhede linked an issue May 29, 2021 that may be closed by this pull request
behavior: animated ? 'smooth' : 'auto',
index: 0,
});
} else {
Copy link

Choose a reason for hiding this comment

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

This can be switched to

const index = inverted ? 0 : vData.length - 1;
virtuosoRef.current?.scrollToIndex({
  behavior: animated ? 'smooth' : 'auto',
  index,           
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks , I will check it out. Planning to publish beta on this next week :) Please let me know if you find more things.
PR needs some testing and some cleanup which will happen early next week :)

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.

I've tried this out over the weekend and put some thoughts here

No hard feelings, but at the moment this web implementation is noticeably slower than the web implementation from react-native-web Flatlist

I've tried playing around with overscan but it doesn't seem to affect the issue

It looks like Virtuoso is the bottleneck though I'm not 100% certain

"react-native": "0.63.4",
"react-native-builder-bob": "^0.17.1",
"react-virtuoso": "^1.6.0",
Copy link

Choose a reason for hiding this comment

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

react-virtuoso should probably be listed in dependencies or peerDependencies

() => ({
flashScrollIndicators: () => null,
getNativeScrollRef: () => null,
getScrollableNode: () => null,
Copy link

Choose a reason for hiding this comment

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

I think this can return scrollerRef.current - getScrollableNode: () => scrollerRef.current

Unfortunately it will be available on the 2nd render, so the parent component that uses BidirectionalFlatList won't have it during componentDidMount or useEffect(() => {}, [])

With the original FlatList getScrollableNode would return the node for componentDidMount

Comment on lines 447 to 465
<Virtuoso<T>
components={{
Footer,
Header,
}}
firstItemIndex={firstItemIndex.current}
initialTopMostItemIndex={
inverted
? vData.length - 1 - initialScrollIndex
: initialScrollIndex
}
itemContent={itemContent}
onScroll={handleScroll}
overscan={300}
ref={virtuosoRef}
// @ts-ignore
scrollerRef={(ref) => (scrollerRef.current = ref)}
totalCount={vData.length}
/>
Copy link

Choose a reason for hiding this comment

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

This portion is pretty much repeated and can be extracted to a const

const VirtuozoNode = (
   <Virtuoso<T> 
       components={{}}
       ...
   />
);

if (!onRefresh) {
  return VirtuozoNode;
}

return (
  <Animated.View>
     <Animated.View>
        <ActivityIndicator />
     </Animated.View>
     
     {VirtuozoNode}
  </Animated.View>
)

<View style={styles.indicatorContainer}>
<ActivityIndicator size={'small'} color={activityIndicatorColor} />
</View>
<Virtuoso<T>
Copy link

Choose a reason for hiding this comment

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

FlatList has getItemLayout prop to aid rendering
Virtuoso has itemSize but it's called with an html element: https://virtuoso.dev/virtuoso-api-reference/ (see itemSize)

I don't see a way to make use of getItemLayout with Virtuoso

  • FlatList.getItemLayout -> is called with data, index and returns { length, offset, index }
  • Virtuoso.itemSize -> is called with domNode and returns a number (height or width of the item)
    • does this mean that Virtuoso have to first render the items and then update the size ?

For web it might be impossible to implement getItemLayout when Virtuoso is used for virtualization

Other libraries like react-virtual provide a compatible interface

You can do something like:

estimateSize = index => {
  const size = props.getItemLayout(props.data, index);
  return size.length; 
}

}
itemContent={itemContent}
onScroll={handleScroll}
overscan={300}
Copy link

Choose a reason for hiding this comment

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

This can be something like

overscan={(scrollerRef.current?.offsetHeight * props.windowSize) || 300}

As per https://reactnative.dev/docs/virtualizedlist#windowsize this is how many screens of data to pre-render
And per https://virtuoso.dev/virtuoso-api-reference/ overscan is in px

Not sure if windowSize has to be halved though

  • for 21 react-native FlatList would add 10 pages up and 10 down
  • would Virtuoso do the same or add 21 pages up and 21 pages down?

}, [onRefresh]);

if (!vData?.length || vData?.length === 0) {
return <ListEmptyComponent />;
Copy link

Choose a reason for hiding this comment

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

It seems that this does not have a default and crashes when vData is empty

Perhaps it can just return null when ListEmptyComponent is not defined
or have a default ListEmptyComponent

Comment on lines +127 to +135
const startReached = async () => {
if (
!onStartReached ||
prependingItems.current ||
onStartReachedInProgress ||
(inverted && !onEndReached) ||
(!inverted && !onStartReached)
) {
return;
Copy link

Choose a reason for hiding this comment

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

For some reason both startReached and endReached are not always triggered, even when there is more data to be loaded

I think it has to do with the count of items rendered
For some chats scrolling to the past (on inverted list) triggers onEndReached and loads older messages, for others it isn't getting picked up
We're using an inverted list so that's where I've test the most, but I did swap it for non-inverted and it seemed it worked better

@vishalnarkhede
Copy link
Contributor Author

I've tried this out over the weekend and put some thoughts here

No hard feelings, but at the moment this web implementation is noticeably slower than the web implementation from react-native-web Flatlist

I've tried playing around with overscan but it doesn't seem to affect the issue

It looks like Virtuoso is the bottleneck though I'm not 100% certain

Hey @kidroca can you share some screen-recording? Just to understand !!

@vishalnarkhede vishalnarkhede force-pushed the vishal/web-support-onrefresh branch from e685877 to c1f1876 Compare July 19, 2021 07:43
@szado
Copy link

szado commented May 27, 2024

@vishalnarkhede do you plan to finish web support? Is this package still being developed?

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.

Does it work on web?
3 participants