Skip to content

virtual-scroll: switch throttleTime to sampleTime #10179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 28, 2018
Merged

virtual-scroll: switch throttleTime to sampleTime #10179

merged 2 commits into from
Feb 28, 2018

Conversation

mmalerba
Copy link
Contributor

sampleTime is actually what I want here since throttleTime could theoretically fail to emit the last scroll event:

throttleTime:

Emits a value from the source Observable, then ignores subsequent source values for duration milliseconds, then repeats this process.

sampleTime:

periodically looks at the source Observable and emits whichever value it has most recently emitted since the previous sampling, unless the source has not emitted anything since the previous sampling.

@mmalerba mmalerba requested a review from crisbeto February 27, 2018 22:28
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 27, 2018
crisbeto
crisbeto approved these changes Feb 28, 2018
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Feb 28, 2018
jelbourn
jelbourn reviewed Feb 28, 2018
View reviewed changes
src/cdk-experimental/scrolling/virtual-scroll-viewport.ts Outdated
@@ -177,7 +177,7 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {

this._ngZone.runOutsideAngular(() => {
fromEvent(this.elementRef.nativeElement, 'scroll')
.pipe(throttleTime(0, animationFrame))
.pipe(sampleTime(0, animationFrame))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment here about what sampleTime is doing?

@mmalerba mmalerba merged commit c5f4db8 into angular:virtual-scroll Feb 28, 2018
mmalerba added a commit that referenced this pull request Mar 7, 2018
@mmalerba
virtual-scroll: switch throttleTime to sampleTime (#10179)
78f2e2a 
* virtual-scroll: switch throttleTime to sampleTime

* add comment
mmalerba added a commit that referenced this pull request Mar 9, 2018
@mmalerba
virtual-scroll: switch throttleTime to sampleTime (#10179)
999f26a 
* virtual-scroll: switch throttleTime to sampleTime

* add comment
mmalerba added a commit that referenced this pull request Mar 19, 2018
@mmalerba
virtual-scroll: switch throttleTime to sampleTime (#10179)
127f4a8 
* virtual-scroll: switch throttleTime to sampleTime

* add comment
mmalerba added a commit to mmalerba/components that referenced this pull request Apr 5, 2018
@mmalerba
virtual-scroll: switch throttleTime to sampleTime (angular#10179)
55da6b2 
* virtual-scroll: switch throttleTime to sampleTime

* add comment
mmalerba added a commit that referenced this pull request May 22, 2018
@mmalerba
virtual-scroll: merge into master under cdk-experimental (#11354)
f59658d 
* 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
@julianobrasil julianobrasil mentioned this pull request Sep 14, 2018
Merged
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion Awaiting requested review from devversion

Assignees

@crisbeto crisbeto

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mmalerba @jelbourn @crisbeto @googlebot