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鈥檒l 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

Conversation

nihgwu
Copy link
Contributor

@nihgwu nihgwu commented Aug 2, 2018

  • api
  • docs
  • tests

馃

</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.)

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?)

@bvaughn
Copy link
Owner

bvaughn commented Aug 3, 2018

Thanks for the PR! Requested some minor changes. 馃槃

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Thanks!

// The estimated total width should be (200 + 40 * 1 + 30 * 4)px = 360px.
expect(scrollContainer.props.style.height).toEqual(540);
expect(scrollContainer.props.style.width).toEqual(360);
});
Copy link
Owner

Choose a reason for hiding this comment

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

This is a much better test! Thank you!

@bvaughn bvaughn merged commit 1aa9f8c into bvaughn:master Aug 4, 2018
@bvaughn
Copy link
Owner

bvaughn commented Aug 4, 2018

Published as 1.1.0

@nihgwu nihgwu deleted the neo/forceUpdate branch August 4, 2018 16:04
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.

None yet

2 participants