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

Allow syncing of scrollLeft and scrollTop from props #624

Merged
merged 2 commits into from
Mar 24, 2017

Conversation

julianwong94
Copy link

MultiGrid manages scrollLeft and scrollTop through state and ignored them through props. Now syncs state if given props.

Modifed componentDidMount and componentWillReceiveProps to sync state if scrollLeft or scrollTop props are passed.

MultiGrid manages scrollLeft and scrollTop through state and ignored them through props. Now syncs state if given props
@@ -144,6 +159,23 @@ export default class MultiGrid extends PureComponent {
this._topGridHeight = null
}

if (
nextProps.scrollLeft !== this.state.scrollLeft ||
nextProps.scrollTop !== this.state.scrollTop
Copy link
Owner

Choose a reason for hiding this comment

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

This should compare nextProps to this.props instead of this.state right? Otherwise, it will interfere with user scrolling.

This kind of half-controlled prop is troublesome. In hindsight, I should have probably used a different approach here. Maybe a public method you call on the ref instead, I dunno.

) {
const newState = {}

if (nextProps.scrollLeft != null) {
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be nextProps.scrollLeft !== this.props.scrollLeft ?

it('should adjust :scrollTop and :scrollLeft when scrollTop and scrollLeft change', () => {
render(getMarkup({
columnWidth: 50,
fixedColumnCount: 2
Copy link
Owner

Choose a reason for hiding this comment

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

These are default values so you can remove them if you want. Either that, or copy them below. Otherwise- if they weren't default values- then the second render would also change columnWidth and fixedColumnCount too (which is probably not what you want, since it potentially gives you a false positive)

Fix to compare nextProps with this.props to not interfere with user scrolling. Modify comparisons to account for negative
@julianwong94
Copy link
Author

I've made the changes to compare nextProps with props instead of state, and fixed the unit test.

@bvaughn
Copy link
Owner

bvaughn commented Mar 24, 2017

Looks good to me. Thanks :)

@bvaughn bvaughn merged commit 120cd3b into bvaughn:master Mar 24, 2017
@bvaughn
Copy link
Owner

bvaughn commented Mar 27, 2017

9.4.0 was just released with this feature. Thank you for your contribution! I listed you in the 9.4.0 release notes.

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.

2 participants