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.

Sign up for GitHub

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

add shouldForceUpdate to resetAfterIndex #32

Merged
merged 4 commits into from
Aug 4, 2018
Merged

add shouldForceUpdate to resetAfterIndex #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b49bab6
add shouldForceUpdate to resetAfterIndex
nihgwu Aug 2, 2018
0ca0c90
address comments
nihgwu Aug 4, 2018
3c83f13
Fixed small mistake in documentation.
bvaughn Aug 4, 2018
dce5b6d
Tightened up Flow types for resetAfterIndices
bvaughn Aug 4, 2018
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
21 changes: 16 additions & 5 deletions src/VariableSizeGrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,20 +396,28 @@ const VariableSizeGrid = createGridComponent({
rowMetadataMap: {},
};

instance.resetAfterColumnIndex = (columnIndex: number) => {
this.resetAfterIndices({ columnIndex });
instance.resetAfterColumnIndex = (
columnIndex: number,
shouldForceUpdate?: boolean = true
) => {
this.resetAfterIndices({ columnIndex, shouldForceUpdate });
};

instance.resetAfterRowIndex = (rowIndex: number) => {
this.resetAfterIndices({ rowIndex });
instance.resetAfterRowIndex = (
rowIndex: number,
shouldForceUpdate?: boolean = true
) => {
this.resetAfterIndices({ rowIndex, shouldForceUpdate });
};

instance.resetAfterIndices = ({
columnIndex,
rowIndex,
shouldForceUpdate = true,
}: {
columnIndex?: number,
rowIndex?: number,
shouldForceUpdate?: boolean,
}) => {
if (typeof columnIndex === 'number') {
instanceProps.lastMeasuredColumnIndex = Math.min(
Expand All @@ -429,7 +437,10 @@ const VariableSizeGrid = createGridComponent({
// It seems an unnecessary optimization.
// It's unlikely that resetAfterIndex() will be called while a user is scrolling.
instance._itemStyleCache = {};
instance.forceUpdate();

if (shouldForceUpdate) {
instance.forceUpdate();
}
};

return instanceProps;
Expand Down
10 changes: 8 additions & 2 deletions src/VariableSizeList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,10 @@ const VariableSizeList = createListComponent({
lastMeasuredIndex: -1,
};

instance.resetAfterIndex = (index: number) => {
instance.resetAfterIndex = (
index: number,
shouldForceUpdate?: boolean = true
) => {
instanceProps.lastMeasuredIndex = Math.min(
instanceProps.lastMeasuredIndex,
index - 1
Expand All @@ -262,7 +265,10 @@ const VariableSizeList = createListComponent({
// It seems an unnecessary optimization.
// It's unlikely that resetAfterIndex() will be called while a user is scrolling.
instance._itemStyleCache = {};
instance.forceUpdate();

if (shouldForceUpdate) {
instance.forceUpdate();
}
};

return instanceProps;
Expand Down
13 changes: 13 additions & 0 deletions src/__tests__/VariableSizeGrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,19 @@ describe('VariableSizeGrid', () => {
});

describe('resetAfterIndex method', () => {
it('should not call forceUpdate if shouldForceUpdate is false', () => {
const rendered = ReactTestRenderer.create(
<VariableSizeGrid {...defaultProps} />
);
rendered.getInstance().forceUpdate = jest.fn();
rendered.getInstance().resetAfterIndices({
columnIndex: 5,
rowIndex: 15,
shouldForceUpdate: false,
});
expect(rendered.getInstance().forceUpdate).toHaveBeenCalledTimes(0);
Copy link
Owner

Choose a reason for hiding this comment

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

I think the way the "should recalculate the estimated total size" test checks for this is a bit more robust. (It checks to see if the columnWidth or rowHeight getter functions have been called.)

forceUpdate is kind of an implementation detail (e.g. maybe the grid re-renders using setState instead?)

});

it('should recalculate the estimated total size', () => {
const columnWidth = jest.fn(() => 75);
const rowHeight = jest.fn(() => 35);
Expand Down
9 changes: 9 additions & 0 deletions src/__tests__/VariableSizeList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,15 @@ describe('VariableSizeList', () => {
});

describe('resetAfterIndex method', () => {
it('should not call forceUpdate if shouldForceUpdate is false', () => {
const rendered = ReactTestRenderer.create(
<VariableSizeList {...defaultProps} />
);
rendered.getInstance().forceUpdate = jest.fn();
rendered.getInstance().resetAfterIndex(15, false);
expect(rendered.getInstance().forceUpdate).toHaveBeenCalledTimes(0);
});

it('should recalculate the estimated total size', () => {
const itemSize = jest.fn(() => 75);
const rendered = ReactTestRenderer.create(
Expand Down
68 changes: 44 additions & 24 deletions website/src/routes/api/VariableSizeGrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,39 +96,59 @@ const PROPS = [
const METHODS = [
{
description: (
<p>
<code>VariableSizeGrid</code> caches offsets and measurements for each
column index for performance purposes. This method clears that cached
data for all columns after (and including) the specified index. It
should be called whenever a column's width changes. (Note that this is
not a typical occurrance.)
</p>
<Fragment>
<p>
<code>VariableSizeGrid</code> caches offsets and measurements for each
column index for performance purposes. This method clears that cached
data for all columns after (and including) the specified index. It
should be called whenever a column's width changes. (Note that this is
not a typical occurrance.)
</p>
<p>
You can set <code>shouldForceUpdate</code> to <code>false</code>
to prevent the list calling <code>forceUpdate</code> internally.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is enough of an explanation. Maybe something more like:

By default the grid will automatically re-render after the index is reset. If you would like to delay this re-render until e.g. a state update has completed in the parent component, specify a value of false for the second, optional parameter.

(And similar for the below blocks.)

</p>
</Fragment>
),
signature: 'resetAfterColumnIndex(index: number): void',
signature:
'resetAfterColumnIndex(index: number, shouldForceUpdate: boolean = true): void',
},
{
description: (
<p>
<code>VariableSizeGrid</code> caches offsets and measurements for each
item for performance purposes. This method clears that cached data for
all items after (and including) the specified indices. It should be
called whenever an items size changes. (Note that this is not a typical
occurrance.)
</p>
<Fragment>
<p>
<code>VariableSizeGrid</code> caches offsets and measurements for each
item for performance purposes. This method clears that cached data for
all items after (and including) the specified indices. It should be
called whenever an items size changes. (Note that this is not a
typical occurrance.)
</p>
<p>
You can set <code>shouldForceUpdate</code> to <code>false</code>
to prevent the list calling <code>forceUpdate</code> internally.
</p>
</Fragment>
),
signature:
'resetAfterIndices({ columnIndex: number, rowIndex: number }): void',
'resetAfterIndices({ columnIndex: number, rowIndex: number, shouldForceUpdate: boolean = true }): void',
},
{
description: (
<p>
<code>VariableSizeGrid</code> caches offsets and measurements for each
row index for performance purposes. This method clears that cached data
for all rows after (and including) the specified index. It should be
called whenever a row's height changes. (Note that this is not a typical
occurrance.)
</p>
<Fragment>
<p>
<code>VariableSizeGrid</code> caches offsets and measurements for each
row index for performance purposes. This method clears that cached
data for all rows after (and including) the specified index. It should
be called whenever a row's height changes. (Note that this is not a
typical occurrance.)
</p>
<p>
You can set <code>shouldForceUpdate</code> to <code>false</code>
to prevent the list calling <code>forceUpdate</code> internally.
</p>
</Fragment>
),
signature: 'resetAfterRowIndex(index: number): void',
signature:
'resetAfterRowIndex(index: number, shouldForceUpdate: boolean = true): void',
},
];
23 changes: 15 additions & 8 deletions website/src/routes/api/VariableSizeList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,21 @@ const PROPS = [
const METHODS = [
{
description: (
<p>
<code>VariableSizeList</code> caches offsets and measurements for each
index for performance purposes. This method clears that cached data for
all items after (and including) the specified index. It should be called
whenever a item's size changes. (Note that this is not a typical
occurrance.)
</p>
<Fragment>
<p>
<code>VariableSizeList</code> caches offsets and measurements for each
index for performance purposes. This method clears that cached data
for all items after (and including) the specified index. It should be
called whenever a item's size changes. (Note that this is not a
typical occurrance.)
</p>
<p>
You can set <code>shouldForceUpdate</code> to <code>false</code>
to prevent the list calling <code>forceUpdate</code> internally.
</p>
</Fragment>
),
signature: 'resetAfterIndex(index: number): void',
signature:
'resetAfterIndex(index: number, shouldForceUpdate: boolean = true): void',
},
];