Scroll to an exact position when clicking the scrollbar track doesn't work in case of variable row heights and isVerticalScrollExact = true #712
Conversation
…ights when scrolling to a y position
…ights when scrolling to a y position
on scrollTo() compute all the row height that were not computed before if isScrollTopExact
pradeepnschrodinger
requested review from
pradeepnschrodinger,
abhisheksingh2410,
karry08 and
AmanGupta2708
src/reducers/computeRenderedRows.js
Outdated
| if (state.isVerticalScrollExact) { | ||
| totalHeight += rowOffsetIntervalTree.get(rowIdx); | ||
| } else { | ||
| totalHeight += updateRowHeight(state, rowIdx); | ||
| } |
There was a problem hiding this comment.
Are we checking for isVerticalScrollExact just to avoid fetching the rows twice in the scenario where initializeRowHeightsAndOffsets() is already called?
If so, initializeRowHeightsAndOffsets() is only called when the no:of total rows or default row height changes.
So the new change effectively can lead to stale row heights for visible rows when isVerticalScrollExact is turned on.
This is a change in behavior because in FDT we always used to update visible row heights on the fly.
IMO, we don't need this change cause we're only re-fetching visible rows, and that's not a big deal in the grand scale of things.
If we really need this micro-optimization, another approach might be to make updateRowHeight() smarter, and have it only fetch the row height once per render. But again, this can be done later.
There was a problem hiding this comment.
yeah i would also suggest to keep the same behaviour and avoid this optimization for now.
There was a problem hiding this comment.
Indeed, the isVerticalScrollExact was just added to avoid fetching the row heights twice.
I have removed this optimization.
Please let me know if it looks ok now.
|
@karry08 , could you take primary here? |
|
LGTM, changes look fine |
|
Fix is available since v2.0.6. |
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: