Skip to content
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.

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

feat(data-table): allow dynamic row heights in data table even when using its virtual scroll #898

Merged
merged 4 commits into from
Oct 2, 2017

Conversation

emoralesb05
Copy link
Contributor

Description

now we will cache the height of every row and recalculate the height as we keep scrolling

Test Steps

General Tests for Every PR

  • ng serve --aot still works.
  • npm run lint passes.
  • npm test passes and code coverage is not lower.
  • npm run build still works.

@@ -73,6 +73,8 @@ export interface IInternalColumnWidth {
max?: boolean;
}

const TD_VIRTUAL_OFFSET: number = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

add comments of what this is

private _totalHeight: number = 0;
private _hostHeight: number = 0;
private _scrollVerticalOffset: number = 0;
private _offsetTransform: SafeStyle;
Copy link
Collaborator

Choose a reason for hiding this comment

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

add comments on what all these are for

@@ -182,6 +156,7 @@ export class TdDataTableComponent implements ControlValueAccessor, OnInit, After

/** internal attributes */
private _data: any[];
private _virtualData: any[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

add comments on what this is

this._resizeSubs = this._onResize.asObservable().subscribe(() => {
if (this._rows) {
this._rows.toArray().forEach((row: TdDataTableRowComponent, index: number) => {
this._rowHeightCache[this.fromRow + index] = row.height + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of caching everything just cache the ones you are looking at. You can recalculate as needed when you scroll up and down. With only a few rows shown should be quick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually just loops through only the ones rendered by the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing we can improve later on is to modify the totalHeight here instead of recalculating it every time.

// and sum them all to calculate the total height
this._data.forEach((d: any, index: number) => {
if (!this._rowHeightCache[index]) {
this._rowHeightCache[index] = this._rowHeightCache[0] || 48;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as comment above about not caching everything

// and sum them all to calculate the total height
this._data.forEach((d: any, index: number) => {
if (!this._rowHeightCache[index]) {
this._rowHeightCache[index] = this._rowHeightCache[0] || 48;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is 48? Maybe put that in a class level variable with a descriptive name and comments describing.

this._totalHeight = 0;
this._fromRow = 0;
this._toRow = 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

lots of calculation going on here, add some comments of what it all is doing.

@emoralesb05 emoralesb05 merged commit 3379024 into develop Oct 2, 2017
@emoralesb05 emoralesb05 deleted the feature/remove-row-height branch October 2, 2017 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants