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

Combine componentWillMount and getInitialState #52

Merged
merged 1 commit into from
Sep 27, 2016
Merged

Combine componentWillMount and getInitialState #52

merged 1 commit into from
Sep 27, 2016

Conversation

jacobbaskin
Copy link
Contributor

Description

There is a bug when setting scrollToRow/scrollToColumn before the initial render of a FixedDataTable: because we compute the state before setting this._rowToScrollTo, we don't scroll appropriately. To fix this bug, and avoid others like it, we combine getInitialState with componentWillMount (which run one after another anyway) into one function and sequence its operation appropriately.

Motivation and Context

Fixes facebook/fixed-data-table/#458

How Has This Been Tested?

Ran 'npm run test'; also tested that this fixed scrollToRow within an internal project.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

Copy link
Contributor

@KamranAsif KamranAsif left a comment

Choose a reason for hiding this comment

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

LGTM. Going to let @wcjordan give this a once over as well

@wcjordan
Copy link
Member

lgtm as well. Thanks!

@KamranAsif KamranAsif merged commit 87f7205 into schrodinger:master Sep 27, 2016
@jacobbaskin
Copy link
Contributor Author

Thanks both of you! When do you next plan to cut a release? Would love to get fixed version from NPM.

@KamranAsif
Copy link
Contributor

Published 0.7.3

@jacobbaskin
Copy link
Contributor Author

Thanks!

@IanVS IanVS mentioned this pull request Sep 30, 2016
7 tasks
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

3 participants