[POC] Bidirectional Flatlist concept #3705
Conversation
It seems this fix is no longer necessary even with the "old" `FlatList`
…te-scroll The `wheel` handler is no longer necessary - scrolling with the mouse wheel works correctly
`onEndReached` callback which triggers `loadMoreChats` expects a promise returning method
The new Flat list is not a class and `instanceOf` fails It uses `useImperativeRef` which passes an object here
| maxToRenderPerBatch={CONST.REPORT.ACTIONS.LIMIT} | ||
| windowSize={CONST.REPORT.ACTIONS.LIMIT / 2} |
There was a problem hiding this comment.
These work better for the new library, or they can also be removed altogether to use the defaults, but maxToRenderPerBatch={1} does not work good with all the updates in this PR
| this.list | ||
| .getScrollableNode() | ||
| .addEventListener('wheel', this.invertedWheelEvent); |
There was a problem hiding this comment.
Scroll is handled automatically from the new library
| this.list.setNativeProps({ | ||
| style: { | ||
| transform: 'translate3d(0,0,0) scaleY(-1)', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This no longer seems needed.
The reason to have it is to fix image rendering, but it does seem to have any effect removing it
scaleY(-1) is added by default
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| <InvertedFlatList {...props} innerRef={ref} /> | ||
| )); | ||
| export default BaseInvertedFlatList; |
There was a problem hiding this comment.
In the end the whole file became a BaseInvertedFlatList with shouldMeasureItems set to true
| @@ -879,6 +879,8 @@ function fetchActions(reportID, offset) { | |||
|
|
|||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, indexedData); | |||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {maxSequenceNumber}); | |||
|
|
|||
| return {indexedData, maxSequenceNumber, history: data.history}; | |||
There was a problem hiding this comment.
Made these changes so that paging can be hooked directly to fetch
It's makes the POC simpler - when we scroll up or down add 25 more messages to component state
Otherwise we need to have some additional handling if data already exists in Onyx
| .sortBy('sequenceNumber') | ||
| .filter(action => action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) |
There was a problem hiding this comment.
the items passed here (from state) are already sorted and filtered
| if (this.waitItemsLayoutTask) { | ||
| this.waitItemsLayoutTask.resolve(); | ||
| this.waitItemsLayoutTask = null; | ||
| this.forceUpdate(); | ||
| } |
There was a problem hiding this comment.
When the action items render we're resolving any pending waitItemsLayoutTask promises
This removes the full screen loader and scrolls to the anchor item
| // Comments have not loaded at all yet do nothing | ||
| if (!_.size(this.props.reportActions)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
removed as we're showing a loader
| <FullScreenLoadingIndicator visible={isLoading} /> | ||
| <InvertedFlatList |
There was a problem hiding this comment.
We're showing this loader for the initial loading that happens during mount
Scrolling up/down uses a small loading indicator provided by default from the library
| reportActions: { | ||
| key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, | ||
| canEvict: false, | ||
| }, |
There was a problem hiding this comment.
This prop is removed so it's easier to test the POC
|
Hey @roryabraham I think we can just close this one |
Details
Don't merge! This is a proof of concept work for the pagination of the Flat List.
Findings
Not ready for WEB
The web implementation has some stopper bugs and is not ready for prime time
onEndReachedto load past messagesOther points
I've removed the prop
reportActionsso that report actions are always fetched and we can see how moving up and down the list loads actions. I can make it work withOnyxdata as well but will make the logic more complicated for review and harder to test the POC.The above change also means that sending a new message would not update the chat.
Bug: When I scroll up to load past messages the chat is sometimes returned all the way to the bottom - I think this is related: GetStream/react-native-bidirectional-infinite-scroll#8 It never happened on scrolling down to load recent messages
I've hooked this up with deeplinking so you can use any past message as the starting point of a report. The message would also have a slightly tinted background so it's easy to trace it (only for review/debug purposes)
Fixed Issues
Related to #2985
Tests
Check for regressions
Test loading relative to an older message
You can trigger this by either triggering a deep link like
expensify-cash://r/{reportID}/{sequenceNumber}or by manually editing the prop passed here:E.g. change it to
1to load every opened chat from the startQA Steps
N/A
Tested On
Screenshots
Web
Does not work very well on web ATM
NEW.Expensify.cash.-.Google.Chrome.2021-06-21.14-27-08.mp4
Mobile Web
Desktop
Android
Android.Bidirectional.Scroll.mp4
iOS
For some reason this popup is constantly triggered when I try to scroll. But overall it functions the same way as in Android
Screen.Recording.2021-06-21.at.14.23.48.mov