Allow syncing of scrollLeft and scrollTop from props #624
Conversation
MultiGrid manages scrollLeft and scrollTop through state and ignored them through props. Now syncs state if given props
source/MultiGrid/MultiGrid.js
Outdated
| @@ -144,6 +159,23 @@ export default class MultiGrid extends PureComponent { | |||
| this._topGridHeight = null | |||
| } | |||
|
|
|||
| if ( | |||
| nextProps.scrollLeft !== this.state.scrollLeft || | |||
| nextProps.scrollTop !== this.state.scrollTop | |||
There was a problem hiding this comment.
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.
source/MultiGrid/MultiGrid.js
Outdated
| ) { | ||
| const newState = {} | ||
|
|
||
| if (nextProps.scrollLeft != null) { |
There was a problem hiding this comment.
This should probably be nextProps.scrollLeft !== this.props.scrollLeft ?
source/MultiGrid/MultiGrid.jest.js
Outdated
| it('should adjust :scrollTop and :scrollLeft when scrollTop and scrollLeft change', () => { | ||
| render(getMarkup({ | ||
| columnWidth: 50, | ||
| fixedColumnCount: 2 |
There was a problem hiding this comment.
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
|
I've made the changes to compare nextProps with props instead of state, and fixed the unit test. |
|
Looks good to me. Thanks :) |
|
9.4.0 was just released with this feature. Thank you for your contribution! I listed you in the 9.4.0 release notes. |
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.