virtual-scroll: simplify scroll listener logic #10102
Conversation
googlebot
added
the
cla: yes
| this._markScrolled(); | ||
| }); | ||
| }); | ||
| fromEvent(this.elementRef.nativeElement, 'scroll').pipe(throttleTime(0, animationFrame)) |
There was a problem hiding this comment.
requestAnimationFrame runs inside the NgZone as well.
There was a problem hiding this comment.
I think this should be ok though. What I was worried about before was that it would trigger change detection for every scroll event, now its throttled by requestAnimationFrame. Even if I subscribe outside the zone, I'm just going to have to go back into the zone and mark for change detection right?
There was a problem hiding this comment.
I'm not familiar with the rxjs internals, but even if it is throttled by requestAnimationFrame, wouldn't it still fire for more or less every scroll event anyway? For the overlay scroll strategies we run everything outside the zone and then its up to the strategy to decide when to re-enter it . Does the virtual scrolling strategy have to re-enter for every pixel or is it only when it swaps out tile? If its the former, then it should be fine to leave it inside the NgZone permanently.
There was a problem hiding this comment.
Hmm that's a good point, let me see if I can rework this. I at least want to get rid of that _markScrolled method
mmalerba
requested review from
andrewseguin,
devversion,
josephperrott,
kara and
tinayuangao
as code owners
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
googlebot
added
cla: no
mmalerba
added
cla: yes
mmalerba
changed the title
|
@crisbeto ok, simplified the scroll listener but left it outside the zone |
mmalerba
requested review from
crisbeto
and removed request for
kara,
devversion,
josephperrott,
tinayuangao and
andrewseguin
crisbeto
added
pr: lgtm
action: merge
* feat(virtual-scroll): fixed size virtual scroll (#9316) * feat(virtual-scroll): fixed size virtual scroll * address some comments * change VirtualScrollStrategy interface a bit * address some more comments * fix lint & build * chore: move virtual-scroll to cdk-experimental (#9974) * virtual-scroll: simplify scroll listener logic (#10102) * virtual-scroll: only move views that need to be moved (#10099) * virtual-scroll: only move views that need to be moved * address comments * virtual-scroll: switch `throttleTime` to `sampleTime` (#10179) * virtual-scroll: switch throttleTime to sampleTime * add comment * virtual-scroll: allow user to pass `Observable<T[]>` (#10158) * virtual-scroll: rename `Range` to `ListRange` to avoid confusion with native `Range` (#10220) * virtual-scroll: add autosize scroll strategy (#10219) * rename fixed size virtual scroll directive * add autosize virtual scroll strategy * add item size estimator class * add logic for jumping rendered content based on scroll position * address comments * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy` (#10504) * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy`. This still has a couple issues that need to be ironed out and it doesn't have the code for correcting the error between the predicted and actual scroll position. (See various TODOs for additional things that need work). * fix lint * address comments * address comments * fix bazel * fix devapp * fix lint * cleanup * virtual-scroll: rewrite offset in terms of "to-top" and fix a bug where items were removed too soon (#10986) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * address comments * virtual-scroll: address amcdnl's feedback (#10988) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * virtual-scroll: address amcdnl's feedback * virtual-scroll: fix updating when data changes and add trackBy demos (#11085) * virtual-scroll: add logic to correct the scroll position as user move… (#11137) * virtual-scroll: add logic to correct the scroll position as user moves toward the top * address comments * fix(scrolling): adds right to fix pushed content (#11192) * fix(scrolling): adds right to fix pushed content * chore: address feedback * chore: address pr feedback * virtual-scroll: add tests for fixed size items (#11255) * virtual-scroll: add tests for fixed size items * address comments * fix bug in fixed size strategy * fix bazel build * virtual-scroll: add tests for `cdkVirtualFor` logic (#11275) * merge fixed size test components into one * add tests for cdkVirtualFor logic * allow undefined to be explicitly passed as trackBy * fix bazel build * address comments * add some basic tests (#11295) * fix lint * virtual-scroll: add e2e tests for autosize scroll strategy (#11345) * set up virtual scroll page in e2e app * add gulp task for e2e:watch * add e2e tests for autosize * address comments * address comments * fix lint and tests * fix bad rebase * fix(scrolling): scrollbar jump when data change on scroll (#11193) * fix(scrolling): data change not updating size * chore: remove todo * chore: more performant fix for jump
|
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. |
This was way more complicated than it needed to be because I was listening to scroll events outside the
NgZone. Instead I'm now subscribing to a version of the scroll events stream that's throttled torequestAnimationFrameto begin with, so no need to leave theNgZone.