React-virtualized: List 8.6.x setting scrollTop breaks scrolling?

[bhj]

Hi, I have a List where I'm setting the initial position on mount via the scrollTop prop. Up through 8.5.3 it works fine, but with 8.6.0 and 8.6.1 no new rows are rendered while scrolling. It seems to be using the static scrollTop prop rather than the current position during a scroll when calculating which rows to render?

[bvaughn]

Potentially related to the 8.6.0 release, specifically PR #482. cc @bradchristensen, @mbrevda

[bvaughn]

I'm at work still but I'll try to make time to look at this this evening. Worst case, I will revert the PR that introduced the regression and then we can work with @bradchristensen at getting it re-submitted.

[bhj]

@bvaughn Have an evening :) and thanks for all your work!

[fritz-c]

I had a similar issue in my react-sortable-tree library at v0.1.7, so if you need to reproduce, just build the example page with npm install && npm start.

I had apparently been setting the scrollTop to something at one point in time with scrollToPixel, but that got phased out, and the broken <= v0.1.7 versions were just constantly passing it scrollTop={null}. Since this was just a programming oversight on my part, I fixed it immediately, and everything was back to normal.

[bradchristensen]

The use case you have would be broken by #482, because what you're doing is setting the component to be controlled (by defining the scrollTop prop) and then expecting it to function in an uncontrolled way from then on (i.e. ignoring the scrollTop prop). With #482, a controlled Grid component will remain controlled. You should find that you can fix this on your end by keeping the scrollTop prop updated with the new scrollTop that you obtain from onScroll. Otherwise, to make the Grid function the way you're currently using it but without it being a controlled component, we would need to introduce a defaultScrollTop prop. (relevant reading)

That being said though, as this is a breaking change for you and others, I think it would be wise to revert it for the 8.x release and re-release it with the next major version. Like @fritz-c mentioned, there are probably others also setting the scrollTop on initial mount without actually making use of it and it would be a shame for this change to break that.

[bhj]

@bradchristensen thanks for the clarification and makes total sense. I'd be happy with a defaultScrollTop or initialScrollTop.

Currently I'm feeding the scrollTop value passed to my onScroll handler back to List, but with Redux in the middle because I need to persist the position in the list across life cycles. So basically there are (throttled) actions periodically updating List's scrollTop and I've just been lucky its been ignoring those during scrolling.

[bvaughn]

That being said though, as this is a breaking change for you and others, I think it would be wise to revert it for the 8.x release and re-release it with the next major version. Like @fritz-c mentioned, there are probably others also setting the scrollTop on initial mount without actually making use of it and it would be a shame for this change to break that.

Agreed. This sounds like the correct way forward.

I will put out a release with this change reverted.

[bvaughn]

I won't revert the whole change set, just the part that impacts controlled vs uncontrolled. There's also a good fix here and coverage from a new test here.

[bvaughn]

Fix released as 8.7.1.

My apologies for the error. Thank you for reporting it so quickly, @bhj. And thanks for chiming in so quickly @bradchristensen and @fritz-c.

[mbrevda]

thanks for the revert @bvaughn! @bradchristensen will you be opening another PR for the next major version with the desired changes?

[bvaughn]

We can come back and reconsider this managed vs unmanaged topic in a few
months. I don't plan a major release anytime before the holidays at a
minimum. :)

On Dec 3, 2016 9:45 PM, "Moshe Brevda" [e-mail] wrote:

thanks for the revert @bvaughn https://github.com/bvaughn!
@bradchristensen https://github.com/bradchristensen will you be opening
another PR for the next major version with the desired changes?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bvaughn/react-virtualized/issues/490#issuecomment-264685905,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznZ4z-If1v79V8RB_tAXfFII65dNUks5rElN8gaJpZM4LDHT5
.

[bradchristensen]

Totally fine by me! Give me a cc when you're next planning a release and I'll be keen to get in there with a PR :smile: