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

Grid: Allow users to opt out of rerendering on scroll stop #1131

Merged
merged 4 commits into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/Grid.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ A windowed grid of elements. `Grid` only renders cells necessary to fill itself
| height | Number | ✓ | Height of Grid; this property determines the number of visible (vs virtualized) rows. |
| id | String | | Optional custom id to attach to root `Grid` element. |
| isScrolling | Boolean | | Override internal is-scrolling state tracking. This property is primarily intended for use with the WindowScroller component. |
| isScrollingOptOut | Boolean | | Prevents re-rendering of visible cells on scroll end. |
| noContentRenderer | Function | | Optional renderer to be rendered inside the grid when either `rowCount` or `columnCount` is empty: `(): PropTypes.node` |
| onSectionRendered | Function | | Callback invoked with information about the section of the Grid that was just rendered. This callback is only invoked when visible rows have changed: `({ columnOverscanStartIndex: number, columnOverscanStopIndex: number, columnStartIndex: number, columnStopIndex: number, rowOverscanStartIndex: number, rowOverscanStopIndex: number, rowStartIndex: number, rowStopIndex: number }): void` |
| onScroll | Function | | Callback invoked whenever the scroll offset changes within the inner scrollable region: `({ clientHeight: number, clientWidth: number, scrollHeight: number, scrollLeft: number, scrollTop: number, scrollWidth: number }): void` |
Expand Down
46 changes: 46 additions & 0 deletions source/Grid/Grid.jest.js
Original file line number Diff line number Diff line change
Expand Up @@ -1905,6 +1905,52 @@ describe('Grid', () => {
expect(cellRendererCalls.length).not.toEqual(0);
});

it('should not clear cache if :isScrollingOptOut is true', () => {
const cellRendererCalls = [];
function cellRenderer({columnIndex, key, rowIndex, style}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as more people start to push perf it might make sense to make some test utils to do things like track render calls rather than a bespoke function in each test. looks great for this issue, but maybe open an issue for that? it's something i may have some time to tackle next week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll open an issue.

cellRendererCalls.push({columnIndex, rowIndex});
return defaultCellRenderer({columnIndex, key, rowIndex, style});
}
const props = {
cellRenderer,
columnWidth: 100,
height: 40,
rowHeight: 20,
scrollTop: 0,
width: 100,
isScrollingOptOut: true,
};

render(getMarkup(props));
render(getMarkup(props));
expect(cellRendererCalls).toEqual([
{columnIndex: 0, rowIndex: 0},
{columnIndex: 0, rowIndex: 1},
]);

cellRendererCalls.splice(0);

render(
getMarkup({
...props,
isScrolling: false,
}),
);

// Visible cells are cached
expect(cellRendererCalls.length).toEqual(0);

render(
getMarkup({
...props,
isScrolling: true,
}),
);

// Only cleared non-visible cells
expect(cellRendererCalls.length).toEqual(0);
});

it('should support a custom :scrollingResetTimeInterval prop', async done => {
const cellRendererCalls = [];
const scrollingResetTimeInterval =
Expand Down
17 changes: 17 additions & 0 deletions source/Grid/Grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,12 @@ type Props = {
*/
isScrolling?: boolean,

/**
* Opt-out of isScrolling param passed to cellRangeRenderer.
* To avoid the extra render when scroll stops.
*/
isScrollingOptOut: boolean,

/** Optional renderer to be used in place of rows when either :rowCount or :columnCount is 0. */
noContentRenderer: NoContentRenderer,

Expand Down Expand Up @@ -276,6 +282,7 @@ class Grid extends React.PureComponent<Props, State> {
scrollToRow: -1,
style: {},
tabIndex: 0,
isScrollingOptOut: false,
};

// Invokes onSectionRendered callback only when start/stop row or column indices change
Expand Down Expand Up @@ -1093,6 +1100,7 @@ class Grid extends React.PureComponent<Props, State> {
overscanRowCount,
rowCount,
width,
isScrollingOptOut,
} = props;

const {
Expand Down Expand Up @@ -1229,6 +1237,7 @@ class Grid extends React.PureComponent<Props, State> {
deferredMeasurementCache,
horizontalOffsetAdjustment,
isScrolling,
isScrollingOptOut,
parent: this,
rowSizeAndPositionManager: instanceProps.rowSizeAndPositionManager,
rowStartIndex,
Expand Down Expand Up @@ -1561,11 +1570,15 @@ class Grid extends React.PureComponent<Props, State> {

_resetStyleCache() {
const styleCache = this._styleCache;
const cellCache = this._cellCache;
const {isScrollingOptOut} = this.props;

// Reset cell and style caches once scrolling stops.
// This makes Grid simpler to use (since cells commonly change).
// And it keeps the caches from growing too large.
// Performance is most sensitive when a user is scrolling.
// Don't clear visible cells from cellCache if isScrollingOptOut is specified.
// This keeps the cellCache to a resonable size.
this._cellCache = {};
this._styleCache = {};

Expand All @@ -1582,6 +1595,10 @@ class Grid extends React.PureComponent<Props, State> {
) {
let key = `${rowIndex}-${columnIndex}`;
this._styleCache[key] = styleCache[key];

if (isScrollingOptOut) {
this._cellCache[key] = cellCache[key];
}
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion source/Grid/defaultCellRangeRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default function defaultCellRangeRenderer({
deferredMeasurementCache,
horizontalOffsetAdjustment,
isScrolling,
isScrollingOptOut,
parent, // Grid (or List or Table)
rowSizeAndPositionManager,
rowStartIndex,
Expand Down Expand Up @@ -109,8 +110,11 @@ export default function defaultCellRangeRenderer({
// However if we are scaling scroll positions and sizes, we should also avoid caching.
// This is because the offset changes slightly as scroll position changes and caching leads to stale values.
// For more info refer to issue #395
//
// If isScrollingOptOut is specified, we always cache cells.
// For more info refer to issue #1028
if (
isScrolling &&
(isScrollingOptOut || isScrolling) &&
!horizontalOffsetAdjustment &&
!verticalOffsetAdjustment
) {
Expand Down
1 change: 1 addition & 0 deletions source/Grid/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export type CellRangeRendererParams = {
deferredMeasurementCache?: Object,
horizontalOffsetAdjustment: number,
isScrolling: boolean,
isScrollingOptOut: boolean,
parent: Object,
rowSizeAndPositionManager: ScalingCellSizeAndPositionManager,
rowStartIndex: number,
Expand Down