Adjust scroll behavior when elements resize #193530
Conversation
amunger
force-pushed
the
aamunger/scrollOnResize
branch
from
4622d4c
to
596d2b2
Compare
|
@joaomoreno - could you take a look at the change to |
| @@ -498,7 +498,7 @@ export class ListView<T> implements IListView<T> { | |||
| this.scrollableElement.delegateVerticalScrollbarPointerDown(browserEvent); | |||
| } | |||
|
|
|||
| updateElementHeight(index: number, size: number | undefined, anchorIndex: number | null): void { | |||
| updateElementHeight(index: number, size: number | undefined, anchorIndex: number | null, renderTopOverride?: number | undefined): void { | |||
There was a problem hiding this comment.
I'm not a huge fan of conflating view model operations (updateElementHeight) with view properties (renderTopOverride). Did you find any other way to solve this?
There was a problem hiding this comment.
In other words, would there be a better default other than Math.max(0, this.lastRenderTop + heightDiff) that would fix this issue and play nicely with all other lists?
There was a problem hiding this comment.
This new behavior will keep the last line in view, and knowing the amount of padding to be able to do that is pretty specific to the type of items in the list, so it would not be straight forward for the listview to take care of this.
Updating element height seems pretty tightly coupled with rendering at a specific render top in the current implementation, but I could look at other options for adjusting those two pieces separately, (like adding the renderTopOverride to rerender?)
There was a problem hiding this comment.
I pushed a commit with an alternative, keeping the scrolltop calculations within the updateElementHeight method. I can clean up the parameters a bit more.
There was a problem hiding this comment.
@rebornix suggested that we go the other way, and make users of updateElementHeight calculate and pass in a render top override if needed. That will simplify listView's updateElementHeight implementation and should allow more explicit control to the users of that function, so I like that idea
joaomoreno
added
the
under-discussion
|
Holding off review until #193231 is stabilized. |
|
closing this for now after the improvements from #194516. |
fix #193231
fix #193835
The previous behavior changed depending on whether the resizing element was only partially visible.
This will drop that variability and instead, just prevent the resizing element from leaving the view entirely.