Skip to content

Commit

Permalink
fix(datagrid): add workaround to enable clrDgSelectable with *ngFor
Browse files Browse the repository at this point in the history
It is impossible (or very difficult) to grab the correct `trackBy`
function via the row iterator structural directive. The `ngForTrackBy`
directive would grab the `trackBy` function from any `*ngFor` in the
datagrid, not just the row iterator.

Further, needing the item index to track selection introduces a race
condition when `*ngFor` is used. We don't have the collection of items
when the rows inputs are set, so `clrDgSelectable` doesn't work.

The fix is to pass a "track by" function directly to the datagrid. This
change introduces a `clrDgItemsTrackBy` input that will used if provided.
Otherwise, the datagrid will fall back to the previous behavior of
trying to grab the `trackBy` function from the row iterator structural
directive. The fallback behavior is deprecated and scheduled for removal
in v17.

This is a backport of 8fe236a to 13.x.

closes #157
  • Loading branch information
kevinbuhmann committed Mar 7, 2023
1 parent a84ce08 commit ccda8c1
Show file tree
Hide file tree
Showing 12 changed files with 599 additions and 178 deletions.
7 changes: 6 additions & 1 deletion projects/angular/clarity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,9 @@ export class ClrDatagrid<T = any> implements AfterContentInit, AfterViewInit, On
// (undocumented)
singleSelectedChanged: EventEmitter<T>;
// (undocumented)
static ɵcmp: i0.ɵɵComponentDeclaration<ClrDatagrid<any>, "clr-datagrid", never, { "loading": "clrDgLoading"; "selected": "clrDgSelected"; "singleSelected": "clrDgSingleSelected"; "clrDgSingleSelectionAriaLabel": "clrDgSingleSelectionAriaLabel"; "clrDgSingleActionableAriaLabel": "clrDgSingleActionableAriaLabel"; "clrDetailExpandableAriaLabel": "clrDetailExpandableAriaLabel"; "clrDgDisablePageFocus": "clrDgDisablePageFocus"; "clrDgPreserveSelection": "clrDgPreserveSelection"; "rowSelectionMode": "clrDgRowSelection"; }, { "refresh": "clrDgRefresh"; "selectedChanged": "clrDgSelectedChange"; "singleSelectedChanged": "clrDgSingleSelectedChange"; }, ["iterator", "placeholder", "columns", "rows"], ["clr-dg-action-bar", "clr-dg-placeholder", "clr-dg-footer", "[clrIfDetail],clr-dg-detail"]>;
set trackBy(value: ClrDatagridItemsTrackByFunction<T>);
// (undocumented)
static ɵcmp: i0.ɵɵComponentDeclaration<ClrDatagrid<any>, "clr-datagrid", never, { "loading": "clrDgLoading"; "selected": "clrDgSelected"; "singleSelected": "clrDgSingleSelected"; "clrDgSingleSelectionAriaLabel": "clrDgSingleSelectionAriaLabel"; "clrDgSingleActionableAriaLabel": "clrDgSingleActionableAriaLabel"; "clrDetailExpandableAriaLabel": "clrDetailExpandableAriaLabel"; "clrDgDisablePageFocus": "clrDgDisablePageFocus"; "clrDgPreserveSelection": "clrDgPreserveSelection"; "rowSelectionMode": "clrDgRowSelection"; "trackBy": "clrDgItemsTrackBy"; }, { "refresh": "clrDgRefresh"; "selectedChanged": "clrDgSelectedChange"; "singleSelectedChanged": "clrDgSingleSelectedChange"; }, ["iterator", "placeholder", "columns", "rows"], ["clr-dg-action-bar", "clr-dg-placeholder", "clr-dg-footer", "[clrIfDetail],clr-dg-detail"]>;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<ClrDatagrid<any>, never>;
}
Expand Down Expand Up @@ -1586,6 +1588,9 @@ export class ClrDatagridItemsTrackBy<T = any> {
static ɵfac: i0.ɵɵFactoryDeclaration<ClrDatagridItemsTrackBy<any>, [{ optional: true; }]>;
}

// @public (undocumented)
export type ClrDatagridItemsTrackByFunction<T> = (item: T) => any;

// @public (undocumented)
export class ClrDatagridModule {
constructor();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ export default function (): void {
});

it('receives an input for the trackBy option', function () {
expect(this.itemsProvider.trackBy).toBeUndefined();
expect(this.itemsProvider.iteratorTrackBy).toBeUndefined();
this.testComponent.trackBy = (index: number) => index;
this.fixture.detectChanges();
expect(this.itemsProvider.trackBy).toBe(this.testComponent.trackBy);
expect(this.itemsProvider.iteratorTrackBy).toBe(this.testComponent.trackBy);
});

it('ignores the column toggle trackBy function', function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class ClrDatagridItemsTrackBy<T = any> {
}

if (this._items) {
this._items.trackBy = value;
this._items.iteratorTrackBy = value;
}
}
}
2 changes: 1 addition & 1 deletion projects/angular/src/data/datagrid/datagrid-items.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export default function (): void {
});

it('items receive the provided trackBy option', function () {
expect(this.clarityDirective.items.trackBy).toBe(this.testComponent.trackBy);
expect(this.clarityDirective.items.iteratorTrackBy).toBe(this.testComponent.trackBy);
});

it('correctly mutates and resets an array with trackBy', function () {
Expand Down
2 changes: 1 addition & 1 deletion projects/angular/src/data/datagrid/datagrid-items.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class ClrDatagridItems<T> implements DoCheck, OnDestroy {

@Input('clrDgItemsTrackBy')
set trackBy(value: TrackByFunction<T>) {
this.items.trackBy = value;
this.items.iteratorTrackBy = value;
this.iterableProxy.ngForTrackBy = value;
}

Expand Down
110 changes: 107 additions & 3 deletions projects/angular/src/data/datagrid/datagrid-row.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@
* The full license information can be found in LICENSE in the root directory of this project.
*/

import { Component } from '@angular/core';
import { fakeAsync, TestBed, tick } from '@angular/core/testing';
import { Component, TrackByFunction } from '@angular/core';
import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing';

import { LoadingListener } from '../../utils/loading/loading-listener';
import { DatagridIfExpandService } from './datagrid-if-expanded.service';
import { ClrDatagridRow } from './datagrid-row';
import { ClrDatagridModule } from './datagrid.module';
import { DatagridDisplayMode } from './enums/display-mode.enum';
import { SelectionType } from './enums/selection-type';
import { DATAGRID_SPEC_PROVIDERS, TestContext } from './helpers.spec';
import { MockDisplayModeService } from './providers/display-mode.mock';
import { DisplayModeService } from './providers/display-mode.service';
import { ExpandableRowsCount } from './providers/global-expandable-rows';
import { Items } from './providers/items';
import { ClrDatagridItemsTrackByFunction, Items } from './providers/items';
import { Selection } from './providers/selection';
import { DatagridRenderOrganizer } from './render/render-organizer';

Expand Down Expand Up @@ -184,6 +185,58 @@ export default function (): void {
});
});

describe('Conditional Selection with *ngFor', () => {
describe('DatagridWithNgForTrackBy', () => {
let fixture: ComponentFixture<NgForDatagridWithNgForTrackBy>;
let nativeElement: HTMLElement;

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ClrDatagridModule],
declarations: [NgForDatagridWithNgForTrackBy],
});

fixture = TestBed.createComponent(NgForDatagridWithNgForTrackBy);
nativeElement = fixture.nativeElement;
});

it('does NOT disable the selection checkbox when clrDgSelectable is false (broken behavior)', () => {
fixture.componentInstance.clrDgSelectable = false;
fixture.detectChanges();

const checkboxElement = nativeElement.querySelector("input[type='checkbox']");

expect(checkboxElement.getAttribute('disabled')).toBeNull();
});
});

[NgForDatagridWithDatagridTrackBy, NgForDatagridWithDatagridTrackByAndNgForTrackBy].forEach(testComponentType => {
describe(testComponentType.name, () => {
let fixture: ComponentFixture<{ clrDgSelectable: boolean }>;
let nativeElement: HTMLElement;

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ClrDatagridModule],
declarations: [testComponentType],
});

fixture = TestBed.createComponent<{ clrDgSelectable: boolean }>(testComponentType);
nativeElement = fixture.nativeElement;
});

it('does disable the selection checkbox when clrDgSelectable is false', () => {
fixture.componentInstance.clrDgSelectable = false;
fixture.detectChanges();

const checkboxElement: HTMLInputElement = nativeElement.querySelector("input[type='checkbox']");

expect(checkboxElement.getAttribute('disabled')).toBeDefined();
});
});
});
});

describe('Selection', function () {
// Until we can properly type "this"
let context: TestContext<ClrDatagridRow, FullTest>;
Expand Down Expand Up @@ -563,3 +616,54 @@ class ExpandTest {
clrDgDetailOpenLabel = 'Open Me';
clrDgDetailCloseLabel = 'Close Me';
}

@Component({
template: `
<clr-datagrid [clrDgSelected]="[]">
<clr-dg-row
*ngFor="let item of items; trackBy: trackBy"
[clrDgItem]="item"
[clrDgSelectable]="clrDgSelectable"
></clr-dg-row>
</clr-datagrid>
`,
})
class NgForDatagridWithNgForTrackBy {
clrDgSelectable = true;

readonly items: Item[] = [{ id: 42 }];
readonly trackBy: TrackByFunction<Item> = (_index, item) => item.id;
}

@Component({
template: `
<clr-datagrid [clrDgSelected]="[]" [clrDgItemsTrackBy]="trackBy">
<clr-dg-row *ngFor="let item of items" [clrDgItem]="item" [clrDgSelectable]="clrDgSelectable"></clr-dg-row>
</clr-datagrid>
`,
})
class NgForDatagridWithDatagridTrackBy {
clrDgSelectable = true;

readonly items: Item[] = [{ id: 42 }];
readonly trackBy: ClrDatagridItemsTrackByFunction<Item> = item => item.id;
}

@Component({
template: `
<clr-datagrid [clrDgSelected]="[]" [clrDgItemsTrackBy]="trackBy">
<clr-dg-row
*ngFor="let item of items; trackBy: iteratorTrackBy"
[clrDgItem]="item"
[clrDgSelectable]="clrDgSelectable"
></clr-dg-row>
</clr-datagrid>
`,
})
class NgForDatagridWithDatagridTrackByAndNgForTrackBy {
clrDgSelectable = true;

readonly items: Item[] = [{ id: 42 }];
readonly datagridTrackBy: ClrDatagridItemsTrackByFunction<Item> = item => item.id;
readonly iteratorTrackBy: TrackByFunction<Item> = (_index, item) => item.id;
}
111 changes: 83 additions & 28 deletions projects/angular/src/data/datagrid/datagrid.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* The full license information can be found in LICENSE in the root directory of this project.
*/

import { ChangeDetectionStrategy, Component, Input, TrackByFunction } from '@angular/core';
import { ChangeDetectionStrategy, Component, Input, TrackByFunction, Type } from '@angular/core';
import { async, fakeAsync, tick } from '@angular/core/testing';
import { Subject } from 'rxjs';

Expand All @@ -25,7 +25,7 @@ import { MockDisplayModeService } from './providers/display-mode.mock';
import { DisplayModeService } from './providers/display-mode.service';
import { FiltersProvider } from './providers/filters';
import { ExpandableRowsCount } from './providers/global-expandable-rows';
import { Items } from './providers/items';
import { ClrDatagridItemsTrackByFunction, Items } from './providers/items';
import { Page } from './providers/page';
import { RowActionService } from './providers/row-action-service';
import { Selection } from './providers/selection';
Expand Down Expand Up @@ -448,13 +448,50 @@ class TabsIntegrationTest {
</clr-datagrid>
`,
})
class PanelTrackByTest {
class PanelIteratorTrackByTest {
items = Array.from(Array(3), (v, i) => {
return { name: v, id: i };
});
trackById: TrackByFunction<{ id: number }> = (index, item) => item.id;
}

@Component({
template: `
<clr-datagrid [clrDgItemsTrackBy]="trackById">
<clr-dg-column>Item</clr-dg-column>
<clr-dg-row *ngFor="let item of items" [clrDgItem]="item">
<clr-dg-cell>{{ item.id }}</clr-dg-cell>
</clr-dg-row>
<clr-dg-detail *clrIfDetail></clr-dg-detail>
</clr-datagrid>
`,
})
class PanelDatagridTrackByTest {
items = Array.from(Array(3), (v, i) => {
return { name: v, id: i };
});
trackById: ClrDatagridItemsTrackByFunction<{ id: number }> = item => item.id;
}

@Component({
template: `
<clr-datagrid [clrDgItemsTrackBy]="datagridTrackById">
<clr-dg-column>Item</clr-dg-column>
<clr-dg-row *ngFor="let item of items; trackBy: iteratorTrackBy" [clrDgItem]="item">
<clr-dg-cell>{{ item.id }}</clr-dg-cell>
</clr-dg-row>
<clr-dg-detail *clrIfDetail></clr-dg-detail>
</clr-datagrid>
`,
})
class PanelDatagridAndIteratorTrackByTest {
items = Array.from(Array(3), (v, i) => {
return { name: v, id: i };
});
datagridTrackById: ClrDatagridItemsTrackByFunction<{ id: number }> = item => item.id;
iteratorTrackBy: TrackByFunction<{ id: number }> = () => Math.random(); // detail pane tracking won't work correctly if this function is used
}

export default function (): void {
describe('ClrDatagrid component', function () {
describe('Typescript API', function () {
Expand Down Expand Up @@ -1232,36 +1269,54 @@ export default function (): void {
});
});

describe('detail pane and track by', function () {
let context: TestContext<ClrDatagrid, PanelTrackByTest>;
let detailService;
interface AbstractDetailPaneTrackByTestComponent {
items: { name: any; id: number }[];
}

beforeEach(function () {
context = this.create(ClrDatagrid, PanelTrackByTest, DATAGRID_SPEC_PROVIDERS);
detailService = context.getClarityProvider(DetailService) as DetailService;
});
function testDetailPaneTrackBy(
description: string,
testComponentType: Type<AbstractDetailPaneTrackByTestComponent>
) {
describe(description, function () {
let context: TestContext<ClrDatagrid, AbstractDetailPaneTrackByTestComponent>;
let detailService;

it('should keep open the panel', () => {
/**
* trackBy function is defined inside the testComponent and
* it is tracking by `id` so unless the id is not changed the
* item must stay the same.
*/
/* Open second detail pane */
const detailButton = context.clarityElement.querySelectorAll('.datagrid-detail-caret-button')[1];
detailService.open(context.testComponent.items[1], detailButton);
context.detectChanges();
beforeEach(function () {
context = this.create(ClrDatagrid, testComponentType, DATAGRID_SPEC_PROVIDERS);
detailService = context.getClarityProvider(DetailService) as DetailService;
});

/* make sure that the state is set */
expect(detailService.state).toEqual(context.testComponent.items[1]);
it('should keep open the panel', () => {
/**
* trackBy function is defined inside the testComponent and
* it is tracking by `id` so unless the id is not changed the
* item must stay the same.
*/
/* Open second detail pane */
const detailButton = context.clarityElement.querySelectorAll('.datagrid-detail-caret-button')[1];
detailService.open(context.testComponent.items[1], detailButton);
context.detectChanges();

/* update the name of the second pane */
context.testComponent.items[1].name = 'changed';
context.detectChanges();
/* make sure that the state is set */
expect(detailService.state).toEqual(context.testComponent.items[1]);

/* update the name of the second pane */
context.testComponent.items[1].name = 'changed';
context.detectChanges();

/* make sure that the same item is still selected */
expect(detailService.state).toEqual(context.testComponent.items[1]);
/* make sure that the same item is still selected */
expect(detailService.state).toEqual(context.testComponent.items[1]);
});
});
});
}

testDetailPaneTrackBy('detail pane and iteratorTrackBy', PanelIteratorTrackByTest);

testDetailPaneTrackBy('detail pane and datagridTrackBy', PanelDatagridTrackByTest);

testDetailPaneTrackBy(
'detail pane and datagridTrackBy (and not iteratorTrackBy)',
PanelDatagridAndIteratorTrackByTest
);
});
}
13 changes: 9 additions & 4 deletions projects/angular/src/data/datagrid/datagrid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import { DetailService } from './providers/detail.service';
import { DisplayModeService } from './providers/display-mode.service';
import { FiltersProvider } from './providers/filters';
import { ExpandableRowsCount } from './providers/global-expandable-rows';
import { Items } from './providers/items';
import { ClrDatagridItemsTrackByFunction, Items } from './providers/items';
import { Page } from './providers/page';
import { RowActionService } from './providers/row-action-service';
import { Selection } from './providers/selection';
Expand Down Expand Up @@ -186,6 +186,11 @@ export class ClrDatagrid<T = any> implements AfterContentInit, AfterViewInit, On
this.selection.rowSelectionMode = value;
}

@Input('clrDgItemsTrackBy')
set trackBy(value: ClrDatagridItemsTrackByFunction<T>) {
this.items.datagridTrackBy = value;
}

/**
* Indicates if all currently displayed items are selected
*/
Expand Down Expand Up @@ -252,9 +257,9 @@ export class ClrDatagrid<T = any> implements AfterContentInit, AfterViewInit, On

// Try to update only when there is something cached and its open.
if (this.detailService.state && this.detailService.isOpen) {
const row = this.rows.find((row, index) => {
return this.items.trackBy(index, row.item) === this.items.trackBy(index, this.detailService.state);
});
const row = this.items.canTrackBy()
? this.rows.find(row => this.items.trackBy(row.item) === this.items.trackBy(this.detailService.state))
: undefined;

/**
* Reopen updated row or close it
Expand Down
2 changes: 2 additions & 0 deletions projects/angular/src/data/datagrid/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ export * from './built-in/comparators/datagrid-property-comparator';

export * from './datagrid.module';

export { ClrDatagridItemsTrackByFunction } from './providers/items';

export { ClrDatagridSelectionCellDirective as ÇlrDatagridSelectionCellDirective } from './datagrid-selection-cell.directive';
export { DatagridDetailRegisterer as ÇlrDatagridDetailRegisterer } from './datagrid-detail-registerer';
export { WrappedCell as ÇlrWrappedCell } from './wrapped-cell';
Expand Down
Loading

0 comments on commit ccda8c1

Please sign in to comment.