feat(virtual-scroll): change fixed strategy to use min & max buffer #12557
Conversation
googlebot
added
the
cla: yes
| if (!this._viewport) { | ||
| return {...range}; | ||
| const firstVisibleIndex = scrollOffset / this._itemSize; | ||
| const range = {...this._viewport.getRenderedRange()}; |
There was a problem hiding this comment.
The spread here transpiles to a for...in loop with some logic in it. Since this method is being while scrolling, it might a good idea if we just constructed the range here ourselves, also consider that it's only two properties.
| range.start = Math.max(0, range.start - expandStart); | ||
| range.end = Math.min(this._viewport.getDataLength(), | ||
| Math.ceil(firstVisibleIndex + | ||
| (this._viewport.getViewportSize() + this._minBufferPx) / this._itemSize)); |
There was a problem hiding this comment.
You're doing a couple of calls to getViewportSize and getDataLength in both the if and else. Consider moving them out into constants.
src/cdk/scrolling/scrolling.md
Outdated
| The fixed size strategy also supports setting the buffer size, i.e. the number of items rendered | ||
| beyond the edge of the viewport. This can be adjusted by setting the `bufferSize` input. If | ||
| `bufferSize` is not specified it defaults to 5 items. | ||
| The fixed size strategy also supports setting a couple buffer parameters that determine how much |
| .subscribe(observer)); | ||
| @Output() scrolledIndexChange: Observable<number> = Observable.create(observer => | ||
| this._scrollStrategy.scrolledIndexChange.subscribe(index => | ||
| Promise.resolve().then(() => this._ngZone.run(() => observer.next(index))))); |
There was a problem hiding this comment.
AFAIK you shouldn't need the NgZone.run here due to the Promise.resolve.
There was a problem hiding this comment.
Is this the case even if scrolledIndexChange emits outside the NgZone? because I think it does
There was a problem hiding this comment.
I confirmed that it does need the .run in this case
|
|
||
| **`minBufferPx`** determines the minimum space outside virtual scrolling viewport that will be | ||
| filled with content. Increasing this will increase the amount of content a user will see before more | ||
| content must be rendered. However, too large a value will cause more content to be rendered than is | ||
| necessary. | ||
|
|
||
| **`addBufferPx`** determines the amount of content that will be added incrementally as the viewport | ||
| **`maxBufferPx`** determines the amount of content that will be added incrementally as the viewport | ||
| is scrolled. This should be greater than the size of `minBufferPx` so that one "render" is needed at |
There was a problem hiding this comment.
Should we throw an error if the minimum is greater than the maximum?
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.