fix: account for ListHeaderComponent length when calculating offset…
#17415
Conversation
… in VirtualizedList - fixes facebook#16612
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@shakyShane I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
react-native-bot
added
Bug Fix 🐛
Ran Commands
facebook-github-bot
added
the
Import Started
There was a problem hiding this comment.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
#17415) Summary: … in VirtualizedList - fixes #16612 Issue is detailed in #16612 <!-- Thank you for sending the PR! We appreciate you spending the time to work on these changes. Help us understand your motivation by explaining why you decided to make this change. You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html Happy contributing! --> I need `onViewableItemsChanged` to account for the length of any ListHeaderComponents I couldn't find any tests that currently cover the use-case of a VirtualizedList + ListComponent + scroll with onViewableItemsChanged, so I have not added any tests - all previous tests pass however. <!-- Help reviewers and the release process by writing your own release notes **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.** CATEGORY [----------] TYPE [ CLI ] [-------------] LOCATION [ DOCS ] [ BREAKING ] [-------------] [ GENERAL ] [ BUGFIX ] [-{Component}-] [ INTERNAL ] [ ENHANCEMENT ] [ {File} ] [ IOS ] [ FEATURE ] [ {Directory} ] |-----------| [ ANDROID ] [ MINOR ] [ {Framework} ] - | {Message} | [----------] [-------------] [-------------] |-----------| [CATEGORY] [TYPE] [LOCATION] - MESSAGE EXAMPLES: [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see --> [GENERAL][BUGFIX][VirtualizedList] - account for `ListHeaderComponent` length Closes #17415 Differential Revision: D8683555 Pulled By: hramos fbshipit-source-id: 05df7b79c16e3c07c12468e782f3c4b0bdce7403
|
FYI, this PR may have caused a rare issue where a FlatList keeps scrolling down without stop. |
|
@bestander Thanks for the update. This is a crucial aspect of creating super-high perf on lists, so I appreciate it now getting some focus :) |
|
Hey all. This is the revert commit 604bcfa. So what happened. We'll be doing a follow up on this issue internally: what was actually causing the infinite scroll and also how to prevent such break in the future. We'll also raise a question of the original issue that this PR tried to fix. @shakyShane, do you have some insight what might have caused the infinite scroll? |
|
We experienced the same infinite scrolling issue when using this patch. Reverting this back or not using a header component fixes the issue. I think the issue occurs when flinging. The |
|
It's so sad that this caused such regressions elsewhere as it's an absolutely essential feature :( if you have complicated lists, windowing is the solution to perf, and when the calculations for |
|
I've just tried the latest React Native (0.58.1) and this is still an issue. :( |
|
Hey @shakyShane I'm facing a issue same as you have mentioned - (#16612). Is there any solution for it? |
… in VirtualizedList - fixes #16612
Issue is detailed in #16612
Motivation
I need
onViewableItemsChangedto account for the length of any ListHeaderComponentsTest Plan
I couldn't find any tests that currently cover the use-case of a VirtualizedList + ListComponent + scroll with onViewableItemsChanged, so I have not added any tests - all previous tests pass however.
Release Notes
[GENERAL][BUGFIX][VirtualizedList] - account for
ListHeaderComponentlength