refactor(grid): Removed scroll throttle for grid scrolls#16871
refactor(grid): Removed scroll throttle for grid scrolls#16871rkaraivanov wants to merge 2 commits intomasterfrom
Conversation
rkaraivanov
added
❌ status: awaiting-test
|
LGTM |
ChronosSF
added
✅ status: verified
There was a problem hiding this comment.
Pull request overview
Removes the grid scroll throttling mechanism (and related test overrides) so scroll notifications are emitted without a throttle delay.
Changes:
- Removed the
SCROLL_THROTTLE_TIMEinjection token and its usage inIgxGridBaseDirective. - Removed
throttleTime(...)from thescrollNotifypipeline. - Simplified multiple grid-related specs by dropping
TestBedprovider overrides for scroll throttling.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/igniteui-angular/grids/grid/src/grid-base.directive.ts | Removes configurable scroll throttling and the throttled scrollNotify RxJS pipeline stage. |
| projects/igniteui-angular/grids/tree-grid/src/tree-grid-summaries.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/tree-grid/src/tree-grid-multi-cell-selection.spec.ts | Removes SCROLL_THROTTLE_TIME test provider overrides in multiple suites. |
| projects/igniteui-angular/grids/tree-grid/src/tree-grid-keyBoardNav.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/hierarchical-grid/src/hierarchical-grid.virtualization.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/hierarchical-grid/src/hierarchical-grid.navigation.spec.ts | Removes SCROLL_THROTTLE_TIME test provider overrides. |
| projects/igniteui-angular/grids/grid/src/grid.master-detail.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/grid/src/grid.component.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/grid/src/grid-mrl-keyboard-nav.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/grid/src/grid-keyBoardNav.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| projects/igniteui-angular/grids/grid/src/grid-cell-selection.spec.ts | Removes SCROLL_THROTTLE_TIME test provider override. |
| import { I18N_FORMATTER } from 'igniteui-angular/core'; | ||
|
|
||
| /** | ||
| * Injection token for setting the throttle time used in grid virtual scroll. | ||
| * @hidden | ||
| */ | ||
| export const SCROLL_THROTTLE_TIME = /*@__PURE__*/new InjectionToken<number>('SCROLL_THROTTLE_TIME', { | ||
| factory: () => 40 | ||
| }); | ||
|
|
||
|
|
||
| interface IMatchInfoCache { | ||
| row: any; |
There was a problem hiding this comment.
Removing the exported SCROLL_THROTTLE_TIME token is a breaking change for any consumers (including internal tooling) that configured grid scroll behavior via DI. Consider keeping the token for backwards compatibility (e.g., deprecated), or reintroducing an equivalent public/hidden configuration point so existing DI configurations don’t hard-break at compile time.
| this.scrollNotify.pipe( | ||
| filter(() => !this._init), | ||
| throttleTime(this.THROTTLE_TIME, animationFrameScheduler, { leading: false, trailing: true }), | ||
| destructor | ||
| ) | ||
| .subscribe((event) => { |
There was a problem hiding this comment.
With throttleTime(...) removed, scrollNotify will now emit on every scroll event, which can be extremely high-frequency and may significantly increase work during scrolling (virtualization, change detection, DOM measurements, etc.). If the goal is to remove the fixed delay but still prevent event floods, consider coalescing emissions to animation frames (e.g., scheduling/coalescing on animationFrameScheduler via an operator like auditTime(0, animationFrameScheduler) or similar) so behavior remains responsive without regressing scroll performance.
|
@ChronosSF I'm against just removing it. Then we'd have almost no performance gain with large number of rows/cells on scroll. I'd prefer if you give more information on the exact scenario you have tested/measured against and the issue you have with the throttle in them so that we may discuss a throttle variation that works for. Some options we have discussed with @rkaraivanov are:
|
3 participants
No description provided.