fix(useVirtualList): List sometimes missing elements #2477
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
begedin
commented
Nov 25, 2022
| if (typeof itemSize === 'number') | ||
| return Math.ceil(containerHeight / itemSize) | ||
| return Math.ceil(containerSize / itemSize) |
There was a problem hiding this comment.
Height naming was from when list was vertical only. Size is more appropriate, IMO
begedin
commented
Nov 25, 2022
Comment on lines
+137
to
+138
| capacity = i | ||
| if (sum > containerSize) |
There was a problem hiding this comment.
Moving capacity out of the if is the fix. Without this, if the original list size is smaller than what could fit in the view, we end up with a capacity of 1 + overscan
There was a problem hiding this comment.
Shouldn't the same thing be done with 'offset = i' in function 'createGetOffset'?
begedin
changed the title
wheatjs
approved these changes
Jan 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2065
This fixes an issue that was closed, seemingly due to inactivity, but was not actually fixed.
This fixes a single scenario I was able to isolate, but I'm not sure if there are actually others. One of the added tests covers the scenario.
Additional context
The scenario I can reliably reproduce the issue in is if the original list is smaller than the capacity of the virtual list. It just doesn't render all the items in that case.
For example, render a list of 5 elements when the screen is able to show 10. With an overscan of 2, 2 + 1 elements will be rendered. With an overscan of 1, it will be 1 + 1 = 2, etc.
Extra notes
It seems that there is still some faulty logic around how overscan works. If you set the overscan to 0, based on tests, you can't really get the first item in the original list to display, possibly the last as well.
I would love to look into that, but it's a bit difficult to unravel what all the start/end variables are intended to be. Not always clear if they are 0 or 1-indexed, inclusive or exclusive, etc.
After this is accepted in, I can look into investigating that and opening a ticket.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).