-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: TableView jump to top on asynchronous load #8133
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! You'll need to sign the CLA for all the checks to pass. If you could provide a minimal example (storybook) for easier verification that would be great. Otherwise, the changes make sense but we'll need to test. |
Let us know if you cannot sign the CLA so we know what action to take. Thank you! |
Hey, thank you for responding! I just signed the CLA (sorry for taking so long). I am also a bit hard-pressed on time to provide a minimal solution, but the bug should become apparent whenever you use paged async loading on a table view and the individual rows are permitted to be of different heights (via And just as a heads-up: We've noticed that while the original issue #8132 is indeed fixed, there are still occasional rendering issues where the table view does not get populated with all loaded rows at the bottom. A workaround for users is to scroll up and down a bit to trigger a re-render which typically fixes it. An deeper investigation is pending on our side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went from a strict object equality to a deep object check, should cause less invalidations
I don't see any specifics to test, so I don't know that it has solved the original issue, right now this appears more as a performance optimization, which I'm fine with.
It might be helpful though to still provide a reproduction in case we have any thoughts as well. Could be useful if there is a race or some unexpected/not anticipated use case involved.
Thanks!
The issue was that since the quality is always wrong (since there are always two distinct objects to compare pointing to different locations in memory) the full re-render resets the vertical scroll positions on data load. This was annoying for users scrolling through a list with several "pages" because loading each page reset their scroll view. |
#8241 may be related with https://codesandbox.io/p/sandbox/lucid-parm-rhyz5n as an example, I'll try replicating locally soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified that it fixes the reproduction shown https://codesandbox.io/p/sandbox/mystifying-shockley-9ws4nq?file=%2Fsrc%2FApp.js%3A65%2C41, so if you'd like you can replicate that as a story.
Closes #8132.
We confirmed that this PR resolves the issue internally, but I will provide a minimal example for easier verification.
✅ Pull Request Checklist:
📝 Test Instructions:
(will provide once minimal example is ready)
🧢 Your Project:
(not applicable)