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

[Table] onVisibleCellsChange not called on scroll in React 16.x #1974

Closed
jdiprizio opened this issue Jan 4, 2018 · 1 comment · Fixed by #2267
Closed

[Table] onVisibleCellsChange not called on scroll in React 16.x #1974

jdiprizio opened this issue Jan 4, 2018 · 1 comment · Fixed by #2267

Comments

@jdiprizio
Copy link

jdiprizio commented Jan 4, 2018

Bug report

  • Package version(s): "@blueprintjs/core": "^1.35.0" "@blueprintjs/table": "^1.31.0"
  • Browser and OS versions: Chrome 63.0.3239.108 Ubuntu 17.10

Steps to reproduce

  1. Add handler with console.log for onVisibleCellsChange on a table with fixed height and lots of rows
  2. Scroll table
  3. Note that handler is called on mount but not on scroll

Actual behavior

onVisibleCellsChange is not called on scroll

Expected behavior

onVisibleCellsChange should be called on scroll

Fix

The issues lies in updateViewportRect method in table.tsx.

Code below:

this.setState({ viewportRect: nextViewportRect });
const { viewportRect } = this.state;

const didViewportChange =
            (viewportRect != null && !viewportRect.equals(nextViewportRect)) ||
            (viewportRect == null && nextViewportRect != null);

is incorrect because viewportRect is destructured after the setState. React recent updates have changed the way setState is executed and state may be set immediately, thus causing didViewportChange to be false and the onVisibleCellsChange to not be invoked. Simply swapping the first two lines fixes the issue.

@giladgray
Copy link
Contributor

@jdiprizio thanks for the bug report, and double thanks for finding the offending code!! would happily accept a PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants