From 3e7aa5f14e0064c5e142b810829019027c52d6eb Mon Sep 17 00:00:00 2001 From: Tony Tan Date: Mon, 17 Apr 2017 18:10:39 -0700 Subject: [PATCH] Fix bug - FlatList component did not render more items when content was filtered Summary: **Bug Description** The FlatList component receives content items via the data prop, and renders an initial number of items on the app's view. When a user scrolls to the end of the list, the component will append and render more available items at the end of the list. There was a bug where when the content was filtered, there were more available items but the component did not append/render them. This is due to the current appending/rendering logic in VirtualizedList, which does not account for data changes as a condition for updating/rendering. VirtualizedList is a dependency of FlatList, so this issue affects FlatList as well. **Approach to Fixing Bug** (i) Reproduce bug on iOS view of FlatList. (ii) For VirtualizedList component: # Isolate onEndReached function that appends more data to component UI. # Isolate _onContentSizeChange function that is called when list content changes. # Write snapshot tests using jest, based off existing test for FlatList. # Refactor logic to append more data to list into _maybeCallOnEndReached function. # Call _maybeCallOnEndReached in both _onContentSizeChange and _onScroll. (iii) Run snapshot tests and observe jest output. (iv) Bring up iOS view of FlatList and check that component now renders more items when content is filtered. Many thanks to sahrens for guidance in developing this code! Reviewed By: sahrens Differential Revision: D4877388 fbshipit-source-id: c10c9eef1912f491450a62b81a9bc41f7f784203 --- Libraries/Lists/VirtualizedList.js | 34 ++- .../Lists/__tests__/VirtualizedList-test.js | 80 ++++++ .../VirtualizedList-test.js.snap | 261 ++++++++++++++++++ 3 files changed, 362 insertions(+), 13 deletions(-) create mode 100644 Libraries/Lists/__tests__/VirtualizedList-test.js create mode 100644 Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index 16a8b5f9d..111d9e2bb 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -623,12 +623,29 @@ class VirtualizedList extends React.PureComponent { return !this.props.horizontal ? metrics.y : metrics.x; } + _maybeCallOnEndReached() { + const {data, getItemCount, onEndReached, onEndReachedThreshold} = this.props; + const {contentLength, visibleLength, offset} = this._scrollMetrics; + const distanceFromEnd = contentLength - visibleLength - offset; + if (onEndReached && + this.state.last === getItemCount(data) - 1 && + distanceFromEnd < onEndReachedThreshold * visibleLength && + (this._hasDataChangedSinceEndReached || + this._scrollMetrics.contentLength !== this._sentEndForContentLength)) { + // Only call onEndReached once for a given dataset + content length. + this._hasDataChangedSinceEndReached = false; + this._sentEndForContentLength = this._scrollMetrics.contentLength; + onEndReached({distanceFromEnd}); + } + } + _onContentSizeChange = (width: number, height: number) => { if (this.props.onContentSizeChange) { this.props.onContentSizeChange(width, height); } this._scrollMetrics.contentLength = this._selectLength({height, width}); this._updateCellsToRenderBatcher.schedule(); + this._maybeCallOnEndReached(); }; _sampleFillRate(sampleType: string) { @@ -662,7 +679,7 @@ class VirtualizedList extends React.PureComponent { const dOffset = offset - this._scrollMetrics.offset; const velocity = dOffset / dt; this._scrollMetrics = {contentLength, dt, offset, timestamp, velocity, visibleLength}; - const {data, getItemCount, onEndReached, onEndReachedThreshold, windowSize} = this.props; + const {data, getItemCount, windowSize} = this.props; this._sampleFillRate('onScroll'); @@ -670,19 +687,10 @@ class VirtualizedList extends React.PureComponent { if (!data) { return; } - const distanceFromEnd = contentLength - visibleLength - offset; - const itemCount = getItemCount(data); - if (onEndReached && - this.state.last === itemCount - 1 && - distanceFromEnd < onEndReachedThreshold * visibleLength && - (this._hasDataChangedSinceEndReached || - this._scrollMetrics.contentLength !== this._sentEndForContentLength)) { - // Only call onEndReached once for a given dataset + content length. - this._hasDataChangedSinceEndReached = false; - this._sentEndForContentLength = this._scrollMetrics.contentLength; - onEndReached({distanceFromEnd}); - } + this._maybeCallOnEndReached(); + const {first, last} = this.state; + const itemCount = getItemCount(data); if ((first > 0 && velocity < 0) || (last < itemCount - 1 && velocity > 0)) { const distanceToContentEdge = Math.min( Math.abs(this._getFrameMetricsApprox(first).offset - offset), diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js new file mode 100644 index 000000000..50ee662d3 --- /dev/null +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -0,0 +1,80 @@ +/** + * Copyright (c) 2015-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + */ +'use strict'; + +jest.disableAutomock(); + +const React = require('React'); +const ReactTestRenderer = require('react-test-renderer'); + +const VirtualizedList = require('VirtualizedList'); + +describe('VirtualizedList', () => { + + it('renders simple list', () => { + const component = ReactTestRenderer.create( + } + /> + ); + expect(component).toMatchSnapshot(); + }); + + it('renders empty list', () => { + const component = ReactTestRenderer.create( + } + /> + ); + expect(component).toMatchSnapshot(); + }); + + it('renders null list', () => { + const component = ReactTestRenderer.create( + } + /> + ); + expect(component).toMatchSnapshot(); + }); + + it('renders all the bells and whistles', () => { + const component = ReactTestRenderer.create( + } + ListFooterComponent={() =>