Virtual scroll perf #737
Virtual scroll perf #737
Conversation
…ual-scroll-perf # Conflicts: # release/components/body/body-cell.component.js # release/components/body/body-cell.component.js.map # release/components/body/body-cell.component.metadata.json # release/components/body/body.component.js # release/components/body/body.component.js.map # release/components/body/body.component.metadata.json # release/components/datatable.component.js # release/components/datatable.component.js.map # release/components/datatable.component.metadata.json # release/index.js # release/index.min.js # release/index.min.js.map
…ual-scroll-perf # Conflicts: # release/components/body/body.component.js.map # release/components/body/body.component.metadata.json # release/components/datatable.component.js # release/components/datatable.component.js.map # release/components/datatable.component.metadata.json # release/index.js # release/index.min.js # release/index.min.js.map
|
I made a few fixes on the path to try to improve perf. I forgot to mention that cpx was broken in the build script. Might be that I am using windows command prompt in vs code. I changed cpx to use quotes instead, otherwise the copy commands were not executed preventing me from building properly. |
| @@ -872,8 +872,7 @@ export class DatatableComponent implements OnInit, AfterViewInit, DoCheck { | |||
| // This is because an expanded row is still considered to be a child of | |||
| // the original row. Hence calculation would use rowHeight only. | |||
| if (this.scrollbarV) { | |||
| const height = typeof this.rowHeight === 'function' ? this.rowHeight() : this.rowHeight; | |||
There was a problem hiding this comment.
Row height can be a function, this removes that ability.
| @@ -1,8 +1,12 @@ | |||
| import { EventEmitter, ElementRef, ViewContainerRef, OnDestroy, AfterViewInit, OnChanges, SimpleChanges, ChangeDetectorRef } from '@angular/core'; | |||
There was a problem hiding this comment.
In the future, please don't include build files.
| @@ -187,12 +198,15 @@ export class DataTableBodyComponent implements OnInit, OnDestroy { | |||
|
|
|||
| rowHeightsCache: RowHeightCache = new RowHeightCache(); | |||
| temp: any[] = []; | |||
| temp2: any[] = []; | |||
There was a problem hiding this comment.
whats the difference between temp and temp2?
There was a problem hiding this comment.
I'm not sure why you are looking at an older change. I should really figure out how to rebase my WIP commits. temp2 was used in the above template (data-selection rows input property) that requires direct raw access to the row data. If you look at https://github.com/swimlane/ngx-datatable/pull/737/files#diff-955af42ecfa71378797f9996448e885aR352 you can see that I map it when it's available. I tried to propagate changes to other components where I could.
| @@ -206,12 +206,11 @@ | |||
|
|
|||
| .progress-linear { | |||
| display: block; | |||
| position: relative; | |||
| position: fixed; | |||
There was a problem hiding this comment.
It's a separate issue I noticed that affected progress indicator scrolling with virtual scroll bar with server side paging. I can open separate issue for this if you'd like. I'm new to this open source thing. First real pull request.
| @@ -0,0 +1,4 @@ | |||
| export interface TrackedRow { | |||
| $$index: number; | |||
| $$expanded: 1 | 0; | |||
There was a problem hiding this comment.
You have to map all rows in order to use expanded property correctly. Since you can do expand all, its required to have a 1-to-1.
There was a problem hiding this comment.
Thanks for the feedback.
|
@amcdnl I ran npm run test and they all passed. I tried to run the e2e commands but they don't seem to work. How do you test the controls? Manually? I was laser focused on my problem set for issues I was having directly and wanted to share my results. I will have more time to investigate sometime early next week. Thanks for all your feedback. This is all very new to me and I don't want to break anything in the process. |
|
Really would like to see this merged soon as IE perf is unusable |
|
@jfinci1 Sorry, I was a bit distracted last week due to Build 2017 and didn't update on my status. I discovered that my pull request broke some supported scenarios. The changes didn't update component variables that changed during scroll and page event handlers that normally occur during change detection. I was exploring ways to manually trigger this after event handler execution. Check out the virtual server side paging demo that changes variables during the handling of the page (scroll) events. https://github.com/swimlane/ngx-datatable/blob/master/demo/paging/paging-virtual.component.ts#L53 @amcdnl The expand-all seems to work just fine on my end. Still working though issues mentioned above. Any ideas on how to best fix this is? Update: I posted the wrong link. |
|
Do you wanna open a new PR w/ these changes given the recent divergence? |
|
yeah, this is a bit out of date and forgotten about. I have some update PR in the works. |
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
Slow virtual scroll due to unnecessary change detection.
Progress indicator scrolls with body.
What is the new behavior?
Faster virtual scroll.
Progress indicator is fixed at top of body.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Renamed scroll event to bodyScroll to avoid an issue in Angular that triggers a change detection. Apparently angular thinks it's a dom scroll event (bug?). Renamed private variables that are listed as public.
Other information:
I'm not sure the impact of this change. Addresses #394 on most perceivable performance issues. Still doesn't address the blank screen when dragging the scroll bar with click and drag motion.