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

Fix: onViewableItemsChanged event handler for inverted FlatList #20

Merged

Conversation

ArekChr
Copy link

@ArekChr ArekChr commented Jul 18, 2023

This pull request solves an issue encountered by @burczu while working on a this proposal. The problem relates to the onViewableItemsChanged event handler, specifically when using an inverted FlatList on the web platform. Currently, the event handler must provide correct results for the list of visible items, leading to inaccurate index determination.

The changes involve adjusting the offset calculation to be in the context of the bottom of the parent element for inverted lists. Implementing these modifications will make the viewableItems calculation accurate for inverted FlatLists on the web platform.

The screenshots highlight the incorrect viewable items in the original implementation and the corrected viewable items after implementing the suggested changes.

Before fix:
Zrzut ekranu 2023-07-18 o 13 30 25

After fix:
Zrzut ekranu 2023-07-18 o 13 31 47

@ArekChr
Copy link
Author

ArekChr commented Jul 19, 2023

cc: @MonilBhavsar

@sobitneupane
Copy link

@MonilBhavsar Changes look good to me.

@ArekChr Can we test it in our App as well?

Looks like the change is introduced to implement this new feature. So, not sure how to test it to see if it serves the purpose.

@ArekChr
Copy link
Author

ArekChr commented Jul 25, 2023

Hey @sobitneupane, testing this in the app might not be possible right now. So, I added a test screen for this purpose. We'll test the issue again once the feature is implemented.

@sobitneupane
Copy link

In that case I think we are good to merge this.

cc: @MonilBhavsar

@MonilBhavsar MonilBhavsar merged commit 1fe5266 into Expensify:master Jul 25, 2023
5 checks passed
@roryabraham
Copy link

Hello everyone 👋🏼

few PSA's here:

  • Generally we should not review or merge any PRs in any of our forked dependencies if there is not an equivalent PR in the upstream. Same goes for any PR that introduces a patch file
  • I think we are going to stop using this fork and move to patch files instead for all the same reasons we ditched the react-native fork. This is easier to build and publish, but still it's an extra step we don't want to deal with and we invested in an awesome new feature in patch-package that allows us to have multiple patch files for a single dependency applied in sequence.
  • In this case, I know necolas does not want any more PRs against VirtualizedList in react-native-web, because the react-native-web lists are basically copy/pasted from react-native with a few custom changes, and he does not want to continue to maintain that code. These lists are in the process of being moved to separate sub-packages of the react-native monorepo: @react-native/virtualized-lists and @react-native/flat-lists, and those sub-packages are to be consumed by both react-native and react-native-web. Progress on that initiative seems to have stalled though

So in conclusion, I think we need to:

  • create an upstream PR against react-native, not react-native-web, for this change
  • Only once that's completed and we have an upstream PR to reference should we proceed with this code in a patch file instead of a fork
  • To prevent any duplicate work, we should wait until [$3500] Upgrade to react-native-web v0.19.6 or greater App#16660 is complete before introducing the new patch including this code

@roryabraham
Copy link

cc @puneetlath

@puneetlath
Copy link

create an upstream PR against react-native, not react-native-web, for this change

@ArekChr would you be down to do this? I'd love to still enable Expensify/App#18101 if possible.

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.

5 participants