-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Make Virtualize behave correctly with when ItemSize is unspecified or wrong. #24920
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
Merged
MackinnonBuck
merged 15 commits into
release/5.0
from
t-mabuc/remove-item-size-dependency
Aug 19, 2020
Merged
Make Virtualize behave correctly with when ItemSize is unspecified or wrong. #24920
MackinnonBuck
merged 15 commits into
release/5.0
from
t-mabuc/remove-item-size-dependency
Aug 19, 2020
Conversation
This file contains hidden or 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
pranavkm
reviewed
Aug 17, 2020
pranavkm
approved these changes
Aug 18, 2020
…com/dotnet/aspnetcore into t-mabuc/remove-item-size-dependency
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.
Summary
With the changes in this PR, users will not have to specify an
ItemSizewhen using<Virtualize>. It is still advantageous to do so, as it can avoid rendering the wrong number of items initially, but has no effect after the initial render.Implementation Details
I've taken a different implementation approach than the one @SteveSandersonMS suggested in #24439 (comment), mostly because it's a nontrivial task to calculate the item height directly, at least from my investigation (one issue being that there could be more than one element in the template, so
offsetHeightof the first element we find might not be enough). Following is an outline of how my implementation works:When parameters are set and the
_itemSizemember is not yet initialized,_itemSizeis initialized to theItemSizeparameter (default is arbitrarily50fif not specified by the user).After each render,
_lastRenderedItemCountand_lastRenderedPlaceholderCountare updated to reflect how many items and placeholders were rendered, respectively.When a spacer becomes visible, a new piece of information is sent to the spacer callbacks -
spacerSeparation- which is the space between the "before" and "after" spacers, calculated fromspacerAfter.offsetTop - (spacerBefore.offsetTop + spacerBefore.offsetHeight).Assuming that placeholder size matches the current
_itemSize1, we can use_lastRenderedItemCountand_lastRenderedPlaceholderCountto calculate how big each item would need to be to fill the remainingspacerSeparation(this is the new_itemSize).Pros
_itemSizeis virtually free since it happens at the same time the spacers are updated).Cons
_itemSizeheight. While it would be nice to find a way to not depend on this, I don't imagine it being a huge problem. The default placeholder always renders the correct size, and placeholders typically have more predictable content, so it should be relatively easy to render custom placeholders at the correct size. When not using anItemsProvider, this isn't even a concern since placeholders don't have to be displayed. I could look into investigating a way for this to work even if users style their placeholder incorrectly, if we deem this to be too big a usability concern._itemSizeis within some ε of the providedItemSize, we stick withItemSize?Other Notes
I think the existing E2E tests validate these improvements. Justification:
RendersWhenItemSizeShrinks_Asyncvalidates that the item count increases when the item size shrinks (the bottom spacer becomes visible, causing a re-render with the new item size). Previously, theItemSizeparameter was used for all calculations, but sinceItemSizeis now used only on the first render,_itemSizemust be properly updated for this test to pass.AlwaysFillsVisibleCapacity_Asyncvalidates that when new items are loaded, each placeholder is replaced with an item, but the total number of rendered list elements does not change. Since_itemSizeis calculated in part by knowing how many placeholders and items are rendered, this test implicitly validates that_lastRenderedItemCountand_lastRenderedPlaceholderCountare calculated and utilized correctly to update_itemSize(if_itemSizechanges after new items were rendered, a different number of elements will render and this test will fail).TODO
Addresses #24439