-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
scroll up an infiniteLoader that has scrollToIndex set bounces back to scrollToIndex #998
Comments
Interested in contributing a new unit test that captures this behavior? Better yet, a new test and a fix? 😄 |
Yes, actually I have a fix (because I hit this in an app I develop). I also looked at the tests, I'm not too sure how to best test this ? Suggestions are welcome 😊
Did
Sent from BlueMail
…On 3 Feb 2018, 17:36, at 17:36, Brian Vaughn ***@***.***> wrote:
Interested in contributing a new unit test that captures this behavior?
Better yet, a new test _and_ a fix? 😄
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#998 (comment)
|
Look at the existing tests. There are many. Should be plenty of examples of testing the individual things your bug report mentions. Comment out your fix. Write a test that fails. Uncomment your fix and make sure the test passes. 🙂 You're obviously welcome to submit just the fix, but it has a better chance of getting merged (and would really help us out) is there was a test too. |
I was asked to write a fix, I did it. I think I played fair. Not very motivating to contribute more. |
@dcolens The reason it was reverted was because the test failed after Its on my list of TODOs. However, if you wan't to take some time to fix the test that is failing that'll be welcome :) |
@wuweiweiwu Hi, it seems like people are too busy to fix this bug. I create #1154 with fixed tests. |
@mengdage awesome! Ill take a look tonight! |
* bvaughn/master: (54 commits) Update version and changelog for 9.21.0 release (bvaughn#1252) chore: update lockfile Update ci badge (bvaughn#1227) Allow users to override default table row styles (bvaughn#1175) Add onColumnClick to Table (bvaughn#1207) remove unused variable in Masonry.example.js (bvaughn#1218) Fix Table aria attributes (bvaughn#1208) Fix typo in CellMeasurer.DynamicHeightTableColumn.example.js (bvaughn#1190) Update usingAutoSizer.md (bvaughn#1186) Add an extra check for an e.target.className.indexOf function (bvaughn#1210) Fix broken Slack badge image (bvaughn#1205) docs(CellMeasurer): fix `import` statement (bvaughn#1187) Added new friend (bvaughn#1197) Fix createMultiSort bug (bvaughn#1051) adding new usecase example and fix some typos (bvaughn#1168) Updating version to 9.20.1 Update changelog for the 9.20.1 release (bvaughn#1167) Prevent early debounceScrollEndedCallback when there is a slow render (bvaughn#1141) removing sideEffects (bvaughn#1163) fix for bvaughn#998 with test cases (bvaughn#1154) ...
the merge was reverted - still having the issue - any workarounds known here? |
Facing the same issue. Any potential workarounds? |
Same issue for me. |
Same issue for me too. |
Experiencing the same issue. Why was the merge reverted? |
For me downgrading to 9.20.0 worked |
Still have this issue InifiteLoader + List in version 9.21.0. @bvaughn is there a walkarounds? |
@bvaughn Facing the same issue with InfinitLoader and Grid with 9.21.2, it would be amazing if this issue could be fixed. |
A tiny workaround I was facing the same issue. I had to render 9k rows in total with a seek functionality for a particular row of interest. After seeking, scrolling up caused no issues, whereas when scrolling down, it automatically jumped to the already seeked row. (I am passing a state parameter to scrollToIndex prop which I am changing so that might rerender the list, but I could not explain why the issue does not come when scrolling up)
I am passing |
repro steps:
some investigation:
It looks very similar to #595:
_recomputeScrollTopFlag is set to true in recomputeGridSize as follow:
I'm suspecting that this code is only valid when scrolling down but not up. If this is correct, recomputeGridSize should use this.state.scrollDirectionVertical to compute _recomputeScrollTopFlag and the same issue likely applies to horizontal scrolling too.
The text was updated successfully, but these errors were encountered: