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

Relative relayouting #1192

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

Relative relayouting #1192

wants to merge 19 commits into from

Conversation

irisjae
Copy link

@irisjae irisjae commented May 8, 2024

Description

When the list enters a region with bad size estimates, scrolling becomes incredibly choppy, appearing to the user as if the list content is jumping around as the user scrolls.

Another issue is that the list is not able to maintain the visible content position when the provided data changes.

This patch implements a relative relayouting algorithm both in native and in RecyclerListView, and solves these problems. Here is a longer analysis of the problem and solution.

Hopefully, this mechanism can be part of FlashList.

Relevant issues:

Relevant PRs:

Reviewers’ hat-rack 🎩

  • Accurate initialRenderIndex
  • Accurate scroll methods
  • Allows list to maintain position across data changes
  • Allows list to not jump when estimated sizes are badly off
  • Allows list to not jump when layouts before visible position change

Screenshots or videos (if needed)

Checklist

@zulfio
Copy link

zulfio commented May 10, 2024

Tested, it's working fine. It's not choppy when scrolling up.

@erquhart
Copy link

@naqvitalha this PR looks to address a major issue for the library. Any chance of it getting reviewed?

@bojandurmic
Copy link

Hi folks, any chance of getting this reviewed and merged? Without this, FlashList is practically useless for chat UIs.

@gelicamarie
Copy link

Any chance we could get this reviewed and merged? This would really help improve our chat feature's UX and resolve scrolling issues. Thanks!

@erquhart
Copy link

erquhart commented Dec 5, 2024

@naqvitalha @davebcn87 this PR has been open for 7 months, and adheres to Shopify's documented contributing requirements. The author provided analysis of underlying issues in their fork readme and commented on several related open issues explaining this. They haven't received any responses that I can find. I know GitHub notifications can pile up, but providing some kind of response to community members that take the time to contribute would be appreciated by everyone, even if you don't merge.

@rklomp
Copy link

rklomp commented Dec 7, 2024

@irisjae Dont know if it helps, but the pull request is still labeled cla-needed

In order to merge this pull request, all contributors must sign Shopify’s CLA.
Sign the CLA and comment "I have signed the CLA!" to re-run the checks and have your PR reviewed.

Copy link

github-actions bot commented Dec 7, 2024

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.

@irisjae
Copy link
Author

irisjae commented Dec 8, 2024

I have signed the CLA!

Copy link

github-actions bot commented Dec 8, 2024

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.

@bojandurmic
Copy link

Hi @irisjae , can you please push an empty commit? It might fix the CLA bot issues 😆

@erquhart
Copy link

erquhart commented Dec 10, 2024

The bot responding with two emoji reactions is hilarious.

Looks like pushing an empty commit did not work.

@erquhart
Copy link

Also probably worthwhile: can everyone following this PR thumb up the original post? Reactions to OP are a common gauge for issue/PR importance to the community.

@hannojg
Copy link

hannojg commented Dec 30, 2024

It seems like this library is using yarn, in this PR npm was used. This is for sure a problem when it comes to merging this (deleted yarn.lock, but added package-lock.json), so just as a friendly heads up: I'd fix this to use yarn again 😅

@bojandurmic
Copy link

Since this is unlikely to ever be merged, I recommend everyone to follow the development of Legend List: https://github.com/LegendApp/legend-list.

It is, of course, very early in development. but the author is an incredibly smart guy and seems to have a lot of motivation to solve every single problem with existing lists.

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

Successfully merging this pull request may close these issues.

7 participants