fix(virtual-core): fixes center alignment option for scrollToOffset #935
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
soren121
changed the title
piecyk
approved these changes
Feb 25, 2025
Collaborator
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Thanks, looks good 👍 When checking the getOffsetForAlignment current we calculate the max offset via reading scrollWidth/scrollHeight, kinda feels we can use the totalSize so just
const maxOffset = this.getTotalSize() - sizeand drop the scrollSize.
|
View your CI Pipeline Execution ↗ for commit 088ca22.
☁️ Nx Cloud last updated this comment at |
Contributor
Author
@piecyk Yeah, that seems like a reasonable improvement to me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #931
This fixes a regression caused by PR #864, which adjusted the center alignment of
scrollToIndexto center on the item. That change moved the center alignment adjustment fromgetOffsetForAlignmentintogetOffsetForIndex. However,scrollToOffsetalso relied on the former function, and now it lacks an implementation foralign: center.This PR restores the center alignment to
getOffsetForAlignment, but adds a newitemSizeparameter. This lets us share the center alignment implementation with both scroll functions, while also taking the item size into account when calculating the center alignment forscrollToIndex.