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.

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

fix(table): eliminate need for second change detection #5775

Merged
merged 4 commits into from
Jul 25, 2017
Merged

fix(table): eliminate need for second change detection #5775

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d643c15
fix(table): eliminate need for second change detection
andrewseguin Jul 14, 2017
15fae92
change var
andrewseguin Jul 14, 2017
982b0ba
move row clear
andrewseguin Jul 14, 2017
1425e19
add comment regarding change det
andrewseguin Jul 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/cdk/table/row.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ export abstract class BaseRowDef {
ngOnChanges(changes: SimpleChanges): void {
// Create a new columns differ if one does not yet exist. Initialize it based on initial value
// of the columns property.
if (!this._columnsDiffer && changes['columns'].currentValue) {
this._columnsDiffer = this._differs.find(changes['columns'].currentValue).create();
const columns = changes['columns'].currentValue;
if (!this._columnsDiffer && columns) {
this._columnsDiffer = this._differs.find(columns).create();
this._columnsDiffer.diff(columns);
}
}

Expand Down
51 changes: 27 additions & 24 deletions src/cdk/table/table.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ describe('CdkTable', () => {
table = component.table;
tableElement = fixture.nativeElement.querySelector('cdk-table');

fixture.detectChanges(); // Let the component and table create embedded views
fixture.detectChanges(); // Let the cells render
fixture.detectChanges();
});

describe('should initialize', () => {
Expand Down Expand Up @@ -120,8 +119,6 @@ describe('CdkTable', () => {
});
});

// TODO(andrewseguin): Add test for dynamic classes on header/rows

it('should use differ to add/remove/move rows', () => {
// Each row receives an attribute 'initialIndex' the element's original place
getRows(tableElement).forEach((row: Element, index: number) => {
Expand Down Expand Up @@ -169,9 +166,7 @@ describe('CdkTable', () => {
dataSource = trackByComponent.dataSource as FakeDataSource;
table = trackByComponent.table;
tableElement = trackByFixture.nativeElement.querySelector('cdk-table');

trackByFixture.detectChanges(); // Let the component and table create embedded views
trackByFixture.detectChanges(); // Let the cells render
trackByFixture.detectChanges();

// Each row receives an attribute 'initialIndex' the element's original place
getRows(tableElement).forEach((row: Element, index: number) => {
Expand Down Expand Up @@ -294,12 +289,10 @@ describe('CdkTable', () => {
});

it('should match the right table content with dynamic data source', () => {
fixture = TestBed.createComponent(DynamicDataSourceCdkTableApp);
component = fixture.componentInstance;
tableElement = fixture.nativeElement.querySelector('cdk-table');

fixture.detectChanges(); // Let the table render the rows
fixture.detectChanges(); // Let the rows render their cells
const dynamicDataSourceFixture = TestBed.createComponent(DynamicDataSourceCdkTableApp);
component = dynamicDataSourceFixture.componentInstance;
tableElement = dynamicDataSourceFixture.nativeElement.querySelector('cdk-table');
dynamicDataSourceFixture.detectChanges();

// Expect that the component has no data source and the table element reflects empty data.
expect(component.dataSource).toBe(undefined);
Expand All @@ -310,10 +303,10 @@ describe('CdkTable', () => {
// Add a data source that has initialized data. Expect that the table shows this data.
const dynamicDataSource = new FakeDataSource();
component.dataSource = dynamicDataSource;
fixture.detectChanges();
dynamicDataSourceFixture.detectChanges();
expect(dynamicDataSource.isConnected).toBe(true);

let data = component.dataSource.data;
const data = component.dataSource.data;
expectTableToMatchContent(tableElement, [
['Column A'],
[data[0].a],
Expand All @@ -323,24 +316,36 @@ describe('CdkTable', () => {

// Remove the data source and check to make sure the table is empty again.
component.dataSource = null;
fixture.detectChanges();
dynamicDataSourceFixture.detectChanges();

// Expect that the old data source has been disconnected.
expect(dynamicDataSource.isConnected).toBe(false);
expectTableToMatchContent(tableElement, [
['Column A']
]);

// Reconnect a data source and check that the table is populated
const newDynamicDataSource = new FakeDataSource();
component.dataSource = newDynamicDataSource;
dynamicDataSourceFixture.detectChanges();
expect(newDynamicDataSource.isConnected).toBe(true);

const newData = component.dataSource.data;
expectTableToMatchContent(tableElement, [
['Column A'],
[newData[0].a],
[newData[1].a],
[newData[2].a],
]);
});

it('should be able to apply classes to rows based on their context', () => {
const contextFixture = TestBed.createComponent(RowContextCdkTableApp);
const contextComponent = contextFixture.componentInstance;
tableElement = contextFixture.nativeElement.querySelector('cdk-table');
contextFixture.detectChanges();

contextFixture.detectChanges(); // Let the table initialize its view
contextFixture.detectChanges(); // Let the table render the rows and cells

const rowElements = contextFixture.nativeElement.querySelectorAll('cdk-row');
let rowElements = contextFixture.nativeElement.querySelectorAll('cdk-row');

// Rows should not have any context classes
for (let i = 0; i < rowElements.length; i++) {
Expand Down Expand Up @@ -374,9 +379,7 @@ describe('CdkTable', () => {
const contextFixture = TestBed.createComponent(RowContextCdkTableApp);
const contextComponent = contextFixture.componentInstance;
tableElement = contextFixture.nativeElement.querySelector('cdk-table');

contextFixture.detectChanges(); // Let the table initialize its view
contextFixture.detectChanges(); // Let the table render the rows and cells
contextFixture.detectChanges();

const rowElements = contextFixture.nativeElement.querySelectorAll('cdk-row');

Expand Down Expand Up @@ -668,7 +671,7 @@ function expectTableToMatchContent(tableElement: Element, expectedTableContent:
// Check data row cells
getRows(tableElement).forEach((row, rowIndex) => {
getCells(row).forEach((cell, cellIndex) => {
const expected = expectedHeaderContent ?
const expected = expectedTableContent.length ?
expectedTableContent[rowIndex][cellIndex] :
null;
checkCellContent(cell, expected);
Expand Down
43 changes: 17 additions & 26 deletions src/cdk/table/table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,11 @@ export class CdkTable<T> implements CollectionViewer {
/** Subject that emits when the component has been destroyed. */
private _onDestroy = new Subject<void>();

/** Flag set to true after the component has been initialized. */
private _isViewInitialized = false;

/** Latest data provided by the data source through the connect interface. */
private _data: NgIterable<T> = [];

/** Subscription that listens for the data provided by the data source. */
private _renderChangeSubscription: Subscription;
private _renderChangeSubscription: Subscription | null;

/**
* Map of all the user's defined columns identified by name.
Expand Down Expand Up @@ -185,6 +182,7 @@ export class CdkTable<T> implements CollectionViewer {
ngOnInit() {
// TODO(andrewseguin): Setup a listener for scroll events
// and emit the calculated view to this.viewChange
this._dataDiffer = this._differs.find([]).create(this._trackByFn);
}

ngAfterContentInit() {
Expand All @@ -209,20 +207,13 @@ export class CdkTable<T> implements CollectionViewer {
this._headerRowPlaceholder.viewContainer.clear();
this._renderHeaderRow();
});
}

ngAfterViewInit() {
// Find and construct an iterable differ that can be used to find the diff in an array.
this._dataDiffer = this._differs.find([]).create(this._trackByFn);
this._isViewInitialized = true;
this._renderHeaderRow();
}

ngDoCheck() {
if (this._isViewInitialized && this.dataSource && !this._renderChangeSubscription) {
this._renderHeaderRow();
if (this.dataSource && !this._renderChangeSubscription) {
this._observeRenderChanges();
}
ngAfterContentChecked() {
if (this.dataSource && !this._renderChangeSubscription) {
this._observeRenderChanges();
}
}

Expand All @@ -234,22 +225,22 @@ export class CdkTable<T> implements CollectionViewer {
private _switchDataSource(dataSource: DataSource<T>) {
this._data = [];

if (this._dataSource) {
if (this.dataSource) {
this.dataSource.disconnect(this);
}
this._dataSource = dataSource;

if (this._isViewInitialized) {
if (this._renderChangeSubscription) {
this._renderChangeSubscription.unsubscribe();
}
// Stop listening for data from the previous data source.
if (this._renderChangeSubscription) {
this._renderChangeSubscription.unsubscribe();
this._renderChangeSubscription = null;
}

if (this._dataSource) {
this._observeRenderChanges();
} else {
this._rowPlaceholder.viewContainer.clear();
}
// Remove the table's rows if there is now no data source
if (!dataSource) {
this._rowPlaceholder.viewContainer.clear();
}

this._dataSource = dataSource;
}

/** Set up a subscription for the data provided by the data source. */
Expand Down