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

The calculations for onViewableItemsChanged are incorrect if Flatlist has ListHeaderComponent #16612

Closed
shakyShane opened this issue Oct 31, 2017 · 3 comments
Assignees
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Resolution: Locked This issue was locked by the bot. Resolution: PR Submitted A pull request with a fix has been provided.

Comments

@shakyShane
Copy link
Contributor

shakyShane commented Oct 31, 2017

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

Environment:
OS: macOS Sierra 10.12.6
Node: 6.9.4
Yarn: 0.19.1
npm: 3.10.10
Watchman: 4.7.0
Xcode: Xcode 9.0 Build version 9A235
Android Studio: 2.3 AI-162.4069837

Packages: (wanted => installed)
react: 16.0.0-beta.5 => 16.0.0-beta.5
react-native: 0.49.5 => 0.49.5

Target Platform: iOS (10.3) (any though)

Steps to Reproduce

  1. Add a Flatlist component
  2. Add a ListHeaderComponent of any length
  3. use onViewableItemsChanged to track viewable rows in local state
  4. That's it, the calculations will be off by the header height.

Expected Behavior

onViewableItemsChanged fires at the correct time

(Write what you thought would happen.)

Actual Behavior

calculations for onViewableItemsChanged are always off by the length of the header.

(Write what happened. Add screenshots!)

Reproducible Demo

switch

Flatlist Bug with ListHeaderComponent

Flatlist exposes onViewableItemsChanged - which can be used to determine which elements are currently
visible within a list. This works correctly.

If a ListHeaderComponent is also given, the calculations used for determining which items are in view
will be incorrect - by exactly the height of the header.

in Libraries/Lists/VirtualizedList.js, this._headerLength is initialised as 0 and then updated on render
to store the length of the header dynamically, but then this value is not used anywhere else.

Calculations need to account for the 'length' of the Header component

We found the easiest way was to subtract the this._headerLength from the offset calculation, such as

_selectOffset(metrics: {x: number, y: number}): number {
-   return !this.props.horizontal ? metrics.y : metrics.x;
+   return (!this.props.horizontal ? metrics.y : metrics.x) - this._headerLength;
}
@shakyShane
Copy link
Contributor Author

android

Sorry I forgot to mention it affects android too

shakyShane added a commit to shakyShane/react-native that referenced this issue Oct 31, 2017
@stale
Copy link

stale bot commented Dec 30, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 30, 2017
shakyShane added a commit to shakyShane/react-native that referenced this issue Jan 2, 2018
shakyShane added a commit to shakyShane/react-native that referenced this issue Jan 2, 2018
shakyShane added a commit to shakyShane/react-native that referenced this issue Jan 2, 2018
shakyShane added a commit to shakyShane/react-native that referenced this issue Jan 2, 2018
@stale stale bot closed this as completed Jan 6, 2018
@esprehn
Copy link
Contributor

esprehn commented Apr 17, 2018

Is it possible to land this? That bug is still there.

@facebook facebook locked and limited conversation to collaborators May 15, 2018
@hramos hramos added Resolution: PR Submitted A pull request with a fix has been provided. 🔶Lists Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. labels Jun 28, 2018
@hramos hramos reopened this Jun 28, 2018
@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 28, 2018
@hramos hramos self-assigned this Jun 28, 2018
hramos pushed a commit that referenced this issue Jul 4, 2018
#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
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jun 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Resolution: Locked This issue was locked by the bot. Resolution: PR Submitted A pull request with a fix has been provided.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants