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

Make isItemViewable handle floating point error. #1029

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tpcstld
Copy link

@tpcstld tpcstld commented Jan 17, 2024

Description

I'm not sure there is an issue for this. I can create one if necessary.

While working on a react-native app, I noticed that some views would never initially appear as visible according to onViewableItemsChanged. From debugging, I isolated the issue to ViewabilityHelper's isItemViewable. https://github.com/Shopify/flash-list/blob/main/src/viewability/ViewabilityHelper.ts#L141

I'm not able to share a video or repro, I'm pretty certain that what I'm seeing is minute differences between pixelsVisible and itemSize. For an example view, I get pixelsVisible = 36.761904761904816 and itemSize = 36.76190476190476. I feel like that these differences are likely caused by floating-errors from prior calculations.

Reviewers’ hat-rack 🎩

  • Do you agree with my hypothesis and reasoning?
  • Should I alternatively do if (pixelsVisible - itemSize >= -0.001)? i.e. allow pixelsVisible to be greater than itemSize in general?
  • Is 0.001 a suitable arbitrary number?

Screenshots or videos (if needed)

Unfortunately I cannot provide this easily. :(

Checklist

@tpcstld
Copy link
Author

tpcstld commented Jan 18, 2024

I will work on getting the CLA signed 😔

@tpcstld
Copy link
Author

tpcstld commented Feb 20, 2024

I have signed the CLA!

Copy link

CLA: Unexpected error has occurred, please re-run the workflow.

We are sorry for the inconvenience, due to GitHub actions limitations this requires a manual intervention.

There are few ways to do it:

  • Create a new pull request with the same changes.
  • Push an empty commit to the branch.
  • Rebase the branch on top of the latest master might also help.

If the issue persists, please contact the maintainers of this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant