From a5694637356de642090ecf5f78bca96372fb8615 Mon Sep 17 00:00:00 2001 From: Kevin Buhmann Date: Tue, 23 Jan 2024 08:50:05 -0600 Subject: [PATCH] feat(datagrid): use only `clrDgItemsTrackBy` for selection tracking (#1144) I removed two tests that appeared to test an optimization to skip work if there are no items. These optimizations are invalid because it may appear that the datagrid has no items when it just doesn't have items yet. CDE-71 BREAKING CHANGE: The row iterator `trackBy` function will no longer be used for selection tracking. Update your code to pass `clrDgItemsTrackBy` to the `clr-datagrid` component if you pass `trackBy` to the row iterator. Note that the `clrDgItemsTrackBy` signature does not include the element index. --- projects/angular/clarity.api.md | 40 +- .../angular/src/data/datagrid/all.spec.ts | 2 - .../datagrid/datagrid-items-trackby.spec.ts | 66 --- .../data/datagrid/datagrid-items-trackby.ts | 35 -- .../src/data/datagrid/datagrid-items.spec.ts | 4 - .../src/data/datagrid/datagrid-items.ts | 1 - .../src/data/datagrid/datagrid-row.spec.ts | 97 +--- .../src/data/datagrid/datagrid.module.ts | 2 - .../src/data/datagrid/datagrid.spec.ts | 111 +--- .../angular/src/data/datagrid/datagrid.ts | 8 +- projects/angular/src/data/datagrid/index.ts | 1 - .../src/data/datagrid/providers/items.ts | 29 +- .../data/datagrid/providers/selection.spec.ts | 512 ++++-------------- .../src/data/datagrid/providers/selection.ts | 36 +- 14 files changed, 191 insertions(+), 753 deletions(-) delete mode 100644 projects/angular/src/data/datagrid/datagrid-items-trackby.spec.ts delete mode 100644 projects/angular/src/data/datagrid/datagrid-items-trackby.ts diff --git a/projects/angular/clarity.api.md b/projects/angular/clarity.api.md index 24ce7cf28b..79230527b2 100644 --- a/projects/angular/clarity.api.md +++ b/projects/angular/clarity.api.md @@ -25,7 +25,7 @@ import { FormGroup } from '@angular/forms'; import { FormGroupDirective } from '@angular/forms'; import { FormGroupName } from '@angular/forms'; import * as i0 from '@angular/core'; -import * as i42 from '@angular/forms'; +import * as i41 from '@angular/forms'; import * as i6 from '@angular/common'; import { InjectionToken } from '@angular/core'; import { Injector } from '@angular/core'; @@ -867,10 +867,10 @@ export class ClrComboboxModule { // Warning: (ae-forgotten-export) The symbol "i4_6" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "i5_4" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "i6_5" needs to be exported by the entry point index.d.ts - // Warning: (ae-forgotten-export) The symbol "i49" needs to be exported by the entry point index.d.ts + // Warning: (ae-forgotten-export) The symbol "i48" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -1550,17 +1550,6 @@ export class ClrDatagridItems implements DoCheck, OnDestroy { static ɵfac: i0.ɵɵFactoryDeclaration, never>; } -// @public (undocumented) -export class ClrDatagridItemsTrackBy { - constructor(_items: Items); - // (undocumented) - set trackBy(value: TrackByFunction); - // (undocumented) - static ɵdir: i0.ɵɵDirectiveDeclaration, "[ngForTrackBy]", never, { "trackBy": "ngForTrackBy"; }, {}, never, never, false, never>; - // (undocumented) - static ɵfac: i0.ɵɵFactoryDeclaration, [{ optional: true; }]>; -} - // @public (undocumented) export type ClrDatagridItemsTrackByFunction = (item: T) => any; @@ -1606,14 +1595,13 @@ export class ClrDatagridModule { // Warning: (ae-forgotten-export) The symbol "i33" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "i34" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "i35" needs to be exported by the entry point index.d.ts - // Warning: (ae-forgotten-export) The symbol "i36" needs to be exported by the entry point index.d.ts + // Warning: (ae-forgotten-export) The symbol "i37" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "i38" needs to be exported by the entry point index.d.ts - // Warning: (ae-forgotten-export) The symbol "i39" needs to be exported by the entry point index.d.ts + // Warning: (ae-forgotten-export) The symbol "i44" needs to be exported by the entry point index.d.ts // Warning: (ae-forgotten-export) The symbol "i45" needs to be exported by the entry point index.d.ts - // Warning: (ae-forgotten-export) The symbol "i46" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -2048,7 +2036,7 @@ export class ClrDatepickerModule { // Warning: (ae-forgotten-export) The symbol "i9_4" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -2499,7 +2487,7 @@ export class ClrInputModule { // Warning: (ae-forgotten-export) The symbol "i2_13" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -2724,7 +2712,7 @@ export class ClrModalModule { // Warning: (ae-forgotten-export) The symbol "i2_23" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -2951,7 +2939,7 @@ export class ClrPasswordModule { // Warning: (ae-forgotten-export) The symbol "i2_14" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -3363,7 +3351,7 @@ export class ClrSelectModule { // Warning: (ae-forgotten-export) The symbol "i2_16" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -3594,7 +3582,7 @@ export class ClrStackViewModule { // Warning: (ae-forgotten-export) The symbol "i5_8" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -3876,7 +3864,7 @@ export class ClrTabsModule { // Warning: (ae-forgotten-export) The symbol "i11_3" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) @@ -3909,7 +3897,7 @@ export class ClrTextareaModule { // Warning: (ae-forgotten-export) The symbol "i2_17" needs to be exported by the entry point index.d.ts // // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; } // @public (undocumented) diff --git a/projects/angular/src/data/datagrid/all.spec.ts b/projects/angular/src/data/datagrid/all.spec.ts index 99a3e8444d..41f63282d4 100644 --- a/projects/angular/src/data/datagrid/all.spec.ts +++ b/projects/angular/src/data/datagrid/all.spec.ts @@ -30,7 +30,6 @@ import DatagridColumnSpecs from './datagrid-column.spec'; import DatagridFilterSpecs from './datagrid-filter.spec'; import DatagridFooterSpecs from './datagrid-footer.spec'; import DatagridHideableColumnDirectiveSpec from './datagrid-hideable-column.spec'; -import DatagridItemsTrackBySpecs from './datagrid-items-trackby.spec'; import DatagridItemsSpecs from './datagrid-items.spec'; import DatagridPageSizeSpecs from './datagrid-page-size.spec'; import DatagridPaginationIntegrationSpecs from './datagrid-pagination.integration.spec'; @@ -82,7 +81,6 @@ describe('Datagrid', function () { DatagridColumnSpecs(); DatagridColumnSeparatorSpecs(); DatagridItemsSpecs(); - DatagridItemsTrackBySpecs(); DatagridRowSpecs(); DatagridRowDetailSpecs(); DatagridPageSizeSpecs(); diff --git a/projects/angular/src/data/datagrid/datagrid-items-trackby.spec.ts b/projects/angular/src/data/datagrid/datagrid-items-trackby.spec.ts deleted file mode 100644 index e776f23625..0000000000 --- a/projects/angular/src/data/datagrid/datagrid-items-trackby.spec.ts +++ /dev/null @@ -1,66 +0,0 @@ -/* - * Copyright (c) 2016-2023 VMware, Inc. All Rights Reserved. - * This software is released under MIT license. - * The full license information can be found in LICENSE in the root directory of this project. - */ - -import { Component, TrackByFunction, ViewChild } from '@angular/core'; -import { TestBed } from '@angular/core/testing'; - -import { columnToggleTrackByFn } from './datagrid-column-toggle-trackby'; -import { ClrDatagridItems } from './datagrid-items'; -import { ClrDatagridModule } from './datagrid.module'; -import { FiltersProvider } from './providers/filters'; -import { Items } from './providers/items'; -import { Page } from './providers/page'; -import { Sort } from './providers/sort'; -import { StateDebouncer } from './providers/state-debouncer.provider'; - -export default function (): void { - describe('DatagridItemsTrackby directive', function () { - beforeEach(function () { - /* - * Since the DatagridItems element is a template that isn't rendered in the DOM, - * we can't use our usual shortcut, we need to rely on @ViewChild - */ - TestBed.configureTestingModule({ - imports: [ClrDatagridModule], - declarations: [FullTest], - providers: [Items, FiltersProvider, Sort, Page, StateDebouncer], - }); - this.fixture = TestBed.createComponent(FullTest); - this.fixture.detectChanges(); - this.testComponent = this.fixture.componentInstance; - this.itemsProvider = TestBed.get(Items); - }); - - afterEach(function () { - this.fixture.destroy(); - }); - - it('receives an input for the trackBy option', function () { - expect(this.itemsProvider.iteratorTrackBy).toBeUndefined(); - this.testComponent.trackBy = (index: number) => index; - this.fixture.detectChanges(); - expect(this.itemsProvider.iteratorTrackBy).toBe(this.testComponent.trackBy); - }); - - it('ignores the column toggle trackBy function', function () { - const initialTrackByFn = this.itemsProvider.trackBy; - this.testComponent.trackBy = columnToggleTrackByFn; - this.fixture.detectChanges(); - expect(this.itemsProvider.trackBy).toBe(initialTrackByFn); - }); - }); -} - -@Component({ - template: `
{{ n }}
`, -}) -class FullTest { - @ViewChild(ClrDatagridItems) datagridItems: ClrDatagridItems; - - numbers = [1, 2, 3, 4, 5]; - - trackBy: TrackByFunction; -} diff --git a/projects/angular/src/data/datagrid/datagrid-items-trackby.ts b/projects/angular/src/data/datagrid/datagrid-items-trackby.ts deleted file mode 100644 index bf6913c65a..0000000000 --- a/projects/angular/src/data/datagrid/datagrid-items-trackby.ts +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright (c) 2016-2023 VMware, Inc. All Rights Reserved. - * This software is released under MIT license. - * The full license information can be found in LICENSE in the root directory of this project. - */ - -import { Directive, Input, Optional, TrackByFunction } from '@angular/core'; - -import { columnToggleTrackByFn } from './datagrid-column-toggle-trackby'; -import { Items } from './providers/items'; - -@Directive({ - selector: '[ngForTrackBy]', -}) -export class ClrDatagridItemsTrackBy { - constructor(@Optional() private _items: Items) {} - - @Input('ngForTrackBy') - set trackBy(value: TrackByFunction) { - /** - * This is a workaround to prevent the items `trackBy` function from - * being replaced when the "manage columns" button is clicked. This is - * not a complete solution. If there is another `ngForTrackBy` function - * within the datagrid in application code, it could sill replace the - * items `trackBy` function whether it is the row iterator or not. - */ - if (value === columnToggleTrackByFn) { - return; - } - - if (this._items) { - this._items.iteratorTrackBy = value; - } - } -} diff --git a/projects/angular/src/data/datagrid/datagrid-items.spec.ts b/projects/angular/src/data/datagrid/datagrid-items.spec.ts index d506491d7d..6f4507a26a 100644 --- a/projects/angular/src/data/datagrid/datagrid-items.spec.ts +++ b/projects/angular/src/data/datagrid/datagrid-items.spec.ts @@ -135,10 +135,6 @@ export default function (): void { expect(this.clarityDirective.iterableProxy.ngForTrackBy).toBe(this.testComponent.trackBy); }); - it('items receive the provided trackBy option', function () { - expect(this.clarityDirective.items.iteratorTrackBy).toBe(this.testComponent.trackBy); - }); - it('correctly mutates and resets an array with trackBy', function () { // Initial state this.fixture.nativeElement.querySelectorAll('li:first-child').forEach(li => (li.style.color = 'red')); diff --git a/projects/angular/src/data/datagrid/datagrid-items.ts b/projects/angular/src/data/datagrid/datagrid-items.ts index ccb11fe2c0..ee80b69f31 100644 --- a/projects/angular/src/data/datagrid/datagrid-items.ts +++ b/projects/angular/src/data/datagrid/datagrid-items.ts @@ -52,7 +52,6 @@ export class ClrDatagridItems implements DoCheck, OnDestroy { @Input('clrDgItemsTrackBy') set trackBy(value: TrackByFunction) { - this.items.iteratorTrackBy = value; this.iterableProxy.ngForTrackBy = value; } diff --git a/projects/angular/src/data/datagrid/datagrid-row.spec.ts b/projects/angular/src/data/datagrid/datagrid-row.spec.ts index 648c2bc851..1a8843c3c0 100644 --- a/projects/angular/src/data/datagrid/datagrid-row.spec.ts +++ b/projects/angular/src/data/datagrid/datagrid-row.spec.ts @@ -4,7 +4,7 @@ * The full license information can be found in LICENSE in the root directory of this project. */ -import { Component, TrackByFunction } from '@angular/core'; +import { Component } from '@angular/core'; import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; import { LoadingListener } from '../../utils/loading/loading-listener'; @@ -185,54 +185,26 @@ export default function (): void { }); describe('Conditional Selection with *ngFor', () => { - describe('DatagridWithNgForTrackBy', () => { - let fixture: ComponentFixture; - 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(); + let fixture: ComponentFixture; + let nativeElement: HTMLElement; - const checkboxElement = nativeElement.querySelector("input[type='checkbox']"); - - expect(checkboxElement.getAttribute('disabled')).toBeNull(); + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [ClrDatagridModule], + declarations: [NgForDatagridWithTrackBy], }); - }); - [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; - }); + fixture = TestBed.createComponent(NgForDatagridWithTrackBy); + nativeElement = fixture.nativeElement; + }); - it('does disable the selection checkbox when clrDgSelectable is false', () => { - fixture.componentInstance.clrDgSelectable = false; - fixture.detectChanges(); + it('does disable the selection checkbox when clrDgSelectable is false', () => { + fixture.componentInstance.clrDgSelectable = false; + fixture.detectChanges(); - const checkboxElement: HTMLInputElement = nativeElement.querySelector("input[type='checkbox']"); + const checkboxElement: HTMLInputElement = nativeElement.querySelector("input[type='checkbox']"); - expect(checkboxElement.getAttribute('disabled')).toBeDefined(); - }); - }); + expect(checkboxElement.getAttribute('disabled')).toBeDefined(); }); }); @@ -632,24 +604,6 @@ class ExpandTest { clrDgDetailCloseLabel = 'Close Me'; } -@Component({ - template: ` - - - - `, -}) -class NgForDatagridWithNgForTrackBy { - clrDgSelectable = true; - - readonly items: Item[] = [{ id: 42 }]; - readonly trackBy: TrackByFunction = (_index, item) => item.id; -} - @Component({ template: ` @@ -657,28 +611,9 @@ class NgForDatagridWithNgForTrackBy { `, }) -class NgForDatagridWithDatagridTrackBy { +class NgForDatagridWithTrackBy { clrDgSelectable = true; readonly items: Item[] = [{ id: 42 }]; readonly trackBy: ClrDatagridItemsTrackByFunction = item => item.id; } - -@Component({ - template: ` - - - - `, -}) -class NgForDatagridWithDatagridTrackByAndNgForTrackBy { - clrDgSelectable = true; - - readonly items: Item[] = [{ id: 42 }]; - readonly datagridTrackBy: ClrDatagridItemsTrackByFunction = item => item.id; - readonly iteratorTrackBy: TrackByFunction = (_index, item) => item.id; -} diff --git a/projects/angular/src/data/datagrid/datagrid.module.ts b/projects/angular/src/data/datagrid/datagrid.module.ts index 16a18a49b2..1154604f37 100644 --- a/projects/angular/src/data/datagrid/datagrid.module.ts +++ b/projects/angular/src/data/datagrid/datagrid.module.ts @@ -53,7 +53,6 @@ import { ClrDatagridFooter } from './datagrid-footer'; import { ClrDatagridHideableColumn } from './datagrid-hideable-column'; import { ClrIfDetail } from './datagrid-if-detail'; import { ClrDatagridItems } from './datagrid-items'; -import { ClrDatagridItemsTrackBy } from './datagrid-items-trackby'; import { ClrDatagridPageSize } from './datagrid-page-size'; import { ClrDatagridPagination } from './datagrid-pagination'; import { ClrDatagridPlaceholder } from './datagrid-placeholder'; @@ -83,7 +82,6 @@ export const CLR_DATAGRID_DIRECTIVES: Type[] = [ ClrDatagridFooter, ClrDatagridHideableColumn, ClrDatagridItems, - ClrDatagridItemsTrackBy, ClrDatagridPageSize, ClrDatagridPagination, ClrDatagridPlaceholder, diff --git a/projects/angular/src/data/datagrid/datagrid.spec.ts b/projects/angular/src/data/datagrid/datagrid.spec.ts index 09bbe89ed6..c61d535932 100644 --- a/projects/angular/src/data/datagrid/datagrid.spec.ts +++ b/projects/angular/src/data/datagrid/datagrid.spec.ts @@ -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, Type } from '@angular/core'; +import { ChangeDetectionStrategy, Component, Input, TrackByFunction } from '@angular/core'; import { async, fakeAsync, tick } from '@angular/core/testing'; import { Subject } from 'rxjs'; @@ -455,25 +455,7 @@ class TabsIntegrationTest { @Component({ template: ` - - Item - - {{ item.id }} - - - - `, -}) -class PanelIteratorTrackByTest { - items = Array.from(Array(3), (v, i) => { - return { name: v, id: i }; - }); - trackById: TrackByFunction<{ id: number }> = (index, item) => item.id; -} - -@Component({ - template: ` - + Item {{ item.id }} @@ -482,32 +464,13 @@ class PanelIteratorTrackByTest { `, }) -class PanelDatagridTrackByTest { +class PanelTrackByTest { items = Array.from(Array(3), (v, i) => { return { name: v, id: i }; }); trackById: ClrDatagridItemsTrackByFunction<{ id: number }> = item => item.id; } -@Component({ - template: ` - - Item - - {{ item.id }} - - - - `, -}) -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 () { @@ -1335,54 +1298,36 @@ export default function (): void { }); }); - interface AbstractDetailPaneTrackByTestComponent { - items: { name: any; id: number }[]; - } - - function testDetailPaneTrackBy( - description: string, - testComponentType: Type - ) { - describe(description, function () { - let context: TestContext; - let detailService; + describe('detail pane and track by', function () { + let context: TestContext; + let detailService; - beforeEach(function () { - context = this.create(ClrDatagrid, testComponentType, DATAGRID_SPEC_PROVIDERS); - detailService = context.getClarityProvider(DetailService) as DetailService; - }); + beforeEach(function () { + context = this.create(ClrDatagrid, PanelTrackByTest, DATAGRID_SPEC_PROVIDERS); + detailService = context.getClarityProvider(DetailService) as 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(); + 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(); - /* make sure that the state is set */ - expect(detailService.state).toEqual(context.testComponent.items[1]); + /* 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(); + /* 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 - ); + }); }); } diff --git a/projects/angular/src/data/datagrid/datagrid.ts b/projects/angular/src/data/datagrid/datagrid.ts index 116d32d358..175aec92d8 100644 --- a/projects/angular/src/data/datagrid/datagrid.ts +++ b/projects/angular/src/data/datagrid/datagrid.ts @@ -212,7 +212,7 @@ export class ClrDatagrid implements AfterContentInit, AfterViewInit, On @Input('clrDgItemsTrackBy') set trackBy(value: ClrDatagridItemsTrackByFunction) { - this.items.datagridTrackBy = value; + this.items.trackBy = value; } /** @@ -266,9 +266,9 @@ export class ClrDatagrid 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.items.canTrackBy() - ? this.rows.find(row => this.items.trackBy(row.item) === this.items.trackBy(this.detailService.state)) - : undefined; + const row = this.rows.find( + row => this.items.trackBy(row.item) === this.items.trackBy(this.detailService.state) + ); /** * Reopen updated row or close it diff --git a/projects/angular/src/data/datagrid/index.ts b/projects/angular/src/data/datagrid/index.ts index da4415d926..4c5f1caa61 100644 --- a/projects/angular/src/data/datagrid/index.ts +++ b/projects/angular/src/data/datagrid/index.ts @@ -18,7 +18,6 @@ export * from './datagrid-hideable-column'; export * from './datagrid-filter'; export * from './datagrid-if-detail'; export * from './datagrid-items'; -export * from './datagrid-items-trackby'; export * from './datagrid-row'; export * from './datagrid-row-detail'; export * from './datagrid-cell'; diff --git a/projects/angular/src/data/datagrid/providers/items.ts b/projects/angular/src/data/datagrid/providers/items.ts index 1f78d2a653..acd845ea82 100644 --- a/projects/angular/src/data/datagrid/providers/items.ts +++ b/projects/angular/src/data/datagrid/providers/items.ts @@ -4,7 +4,7 @@ * The full license information can be found in LICENSE in the root directory of this project. */ -import { Injectable, TrackByFunction } from '@angular/core'; +import { Injectable } from '@angular/core'; import { Observable } from 'rxjs'; import { Subject } from 'rxjs'; import { Subscription } from 'rxjs'; @@ -22,11 +22,6 @@ export class Items { */ loading = false; - /** - * New tracking function to identify objects. If provided, this will be used instead of `iteratorTrackBy`. - */ - datagridTrackBy: ClrDatagridItemsTrackByFunction; - /** * Subscriptions to the other providers changes. */ @@ -104,11 +99,9 @@ export class Items { } /** - * Tracking function to identify objects. Default is reference equality. - * - * @deprecated in v15 and scheduled for removal in v17 (CDE-71) + * Tracking function to identify objects. */ - iteratorTrackBy: TrackByFunction = (_index, item) => item; + trackBy: ClrDatagridItemsTrackByFunction = item => item; /** * Cleans up our subscriptions to other providers @@ -153,22 +146,6 @@ export class Items { } } - canTrackBy(): boolean { - // all items are needed unless `datagridTrackBy` is set because `iteratorTrackBy` requires the item's index - return !!this.datagridTrackBy || Array.isArray(this.all); - } - - trackBy(item: T, index?: number) { - if (this.datagridTrackBy) { - return this.datagridTrackBy(item); - } else if (Array.isArray(this.all)) { - index = index ?? this.all.indexOf(item); - return this.iteratorTrackBy(index, item); - } else { - throw new Error('improper call to Items#trackBy'); - } - } - private emitChange() { this._change.next(this.displayed); } diff --git a/projects/angular/src/data/datagrid/providers/selection.spec.ts b/projects/angular/src/data/datagrid/providers/selection.spec.ts index f1ec65ffb2..d12b79e2f4 100644 --- a/projects/angular/src/data/datagrid/providers/selection.spec.ts +++ b/projects/angular/src/data/datagrid/providers/selection.spec.ts @@ -503,49 +503,15 @@ export default function (): void { expect(selectionInstance.current).toEqual([items[2]]); }); - function testTrackBy(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - selectionInstance.setSelected(items[2], true); - const clones = cloneItems(); - itemsInstance.all = clones; - tick(); - expect(selectionInstance.current).toEqual([clones[2]]); - testSelectedItems(clones, [2]); - }); - } - - it( - 'should support iteratorTrackBy item id', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'should support iteratorTrackBy index', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = index => index; - }) - ); - - it( - 'should support datagridTrackBy item id', - testTrackBy(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'should use datagridTrackBy instead of iteratorTrackBy', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); + it('should support trackBy item id', fakeAsync(function () { + itemsInstance.trackBy = item => item.id; + selectionInstance.setSelected(items[2], true); + const clones = cloneItems(); + itemsInstance.all = clones; + tick(); + expect(selectionInstance.current).toEqual([clones[2]]); + testSelectedItems(clones, [2]); + })); }); describe('single selection', function () { @@ -572,60 +538,14 @@ export default function (): void { expect(selectionInstance.currentSingle).toBe(undefined); })); - it('does not apply iteratorTrackBy to single selection with no items', () => { - const emptyItems = new Items(filtersInstance, sortInstance, pageInstance); - const selection = new Selection(emptyItems, filtersInstance); - - spyOn(emptyItems, 'iteratorTrackBy'); - - expect(selection.currentSingle).toBeUndefined(); - selection.currentSingle = items[2]; - - expect(emptyItems.iteratorTrackBy).not.toHaveBeenCalled(); - }); - - function testTrackBy(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - selectionInstance.currentSingle = items[2]; - const clones = cloneItems(); - itemsInstance.all = clones; - tick(); - testSelectedItems(clones, [2]); - }); - } - - it( - 'should support iteratorTrackBy item id', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'should support iteratorTrackBy index', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = index => index; - }) - ); - - it( - 'should support datagridTrackBy item id', - testTrackBy(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'should use datagridTrackBy instead of iteratorTrackBy', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); + it('should support trackBy item id', fakeAsync(function () { + itemsInstance.trackBy = item => item.id; + selectionInstance.currentSingle = items[2]; + const clones = cloneItems(); + itemsInstance.all = clones; + tick(); + testSelectedItems(clones, [2]); + })); }); }); @@ -678,176 +598,67 @@ export default function (): void { selectionInstance.selectionType = SelectionType.Multi; }); // We don't support server-driven, multi-selection without trackBy. - // We don't support server-driven, multi-selection, iteratorTrackBy index. - - function testTrackBy(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - itemsInstance.all = itemsA; - tick(); - selectionInstance.setSelected(itemsA[0], true); - testSelection(true, false, false); - itemsInstance.all = itemsB; - tick(); - testSelection(true, false, false); - itemsInstance.all = itemsC; - tick(); - testSelection(false, false, true); - expect(selectionInstance.current[0].modified).toEqual(true); - }); - } - - it( - 'should support iteratorTrackBy item id', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'should support datagridTrackBy item id', - testTrackBy(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'should use datagridTrackBy instead of iteratorTrackBy', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - function testTrackByPreselectedItems(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - selectionInstance.current = [{ id: 1 }, { id: 2 }, { id: 3 }]; - tick(); - itemsInstance.all = itemsA; - tick(); - itemsA.forEach(item => { - expect(selectionInstance.isSelected(item)).toBe(true); - }); - }); - } - - it( - 'accepts pre-selected items with iteratorTrackBy when `all` has not been defined', - testTrackByPreselectedItems(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'accepts pre-selected items with datagridTrackBy when `all` has not been defined', - testTrackByPreselectedItems(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'accepts pre-selected items with datagridTrackBy (and not iteratorTrackBy) when `all` has not been defined', - testTrackByPreselectedItems(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - function testTrackByToggleAll(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - itemsInstance.all = itemsA; - pageInstance.size = 3; - pageInstance.current = 1; - tick(); - selectionInstance.toggleAll(); - testToggleAllSelection(true, false, false); - itemsInstance.all = itemsB; - pageInstance.current = 2; - tick(); - testToggleAllSelection(true, false, false); - itemsInstance.all = itemsC; - pageInstance.current = 1; - tick(); - testToggleAllSelection(false, false, true); - expect(selectionInstance.current[0].modified).toEqual(true); - }); - } - - it( - 'should support toggleAll selection on page change with iteratorTrackBy', - testTrackByToggleAll(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'should support toggleAll selection on page change with datagridTrackBy', - testTrackByToggleAll(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'should support toggleAll selection on page change with datagridTrackBy (and not iteratorTrackBy)', - testTrackByToggleAll(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - function testTrackByPreserveSelection(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - const evenFilter: ItemEvenFilter = new ItemEvenFilter(); - itemsInstance.all = itemsA; - tick(); - selectionInstance.setSelected(itemsA[0], true); - selectionInstance.preserveSelection = true; - - filtersInstance.add(evenFilter as ClrDatagridFilterInterface); - evenFilter.toggle(); - tick(); - - expect(selectionInstance.current.length).toBe(1); - expect(selectionInstance.isSelected(itemsA[0])).toBeTruthy(); + // We don't support server-driven, multi-selection, trackBy index. + it('should support trackBy item id', fakeAsync(() => { + itemsInstance.trackBy = item => item.id; + itemsInstance.all = itemsA; + tick(); + selectionInstance.setSelected(itemsA[0], true); + testSelection(true, false, false); + itemsInstance.all = itemsB; + tick(); + testSelection(true, false, false); + itemsInstance.all = itemsC; + tick(); + testSelection(false, false, true); + expect(selectionInstance.current[0].modified).toEqual(true); + })); + + it('accepts pre-selected items with trackBy when `all` has not been defined', fakeAsync(() => { + itemsInstance.trackBy = item => item.id; + selectionInstance.current = [{ id: 1 }, { id: 2 }, { id: 3 }]; + tick(); + itemsInstance.all = itemsA; + tick(); + itemsA.forEach(item => { + expect(selectionInstance.isSelected(item)).toBe(true); }); - } - - it( - 'should not clear selection when filter applied and clrDgPreserveSelection is true with iteratorTrackBy', - testTrackByPreserveSelection(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'should not clear selection when filter applied and clrDgPreserveSelection is true with datagridTrackBy', - testTrackByPreserveSelection(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'should not clear selection when filter applied and clrDgPreserveSelection is true with datagridTrackBy (and not iteratorTrackBy)', - testTrackByPreserveSelection(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); + })); + + it('should support toggleAll selection on page change', fakeAsync(() => { + itemsInstance.trackBy = item => item.id; + itemsInstance.all = itemsA; + pageInstance.size = 3; + pageInstance.current = 1; + tick(); + selectionInstance.toggleAll(); + testToggleAllSelection(true, false, false); + itemsInstance.all = itemsB; + pageInstance.current = 2; + tick(); + testToggleAllSelection(true, false, false); + itemsInstance.all = itemsC; + pageInstance.current = 1; + tick(); + testToggleAllSelection(false, false, true); + expect(selectionInstance.current[0].modified).toEqual(true); + })); + + it('should not clear selection when filter applied and clrDgPreserveSelection is true', fakeAsync(() => { + const evenFilter: ItemEvenFilter = new ItemEvenFilter(); + itemsInstance.trackBy = item => item.id; + itemsInstance.all = itemsA; + tick(); + selectionInstance.setSelected(itemsA[0], true); + selectionInstance.preserveSelection = true; + + filtersInstance.add(evenFilter as ClrDatagridFilterInterface); + evenFilter.toggle(); + tick(); + + expect(selectionInstance.current.length).toBe(1); + expect(selectionInstance.isSelected(itemsA[0])).toBeTruthy(); + })); }); describe('single selection', function () { @@ -855,59 +666,24 @@ export default function (): void { selectionInstance.selectionType = SelectionType.Single; }); // We don't support server-driven, multi-selection without trackBy. - // We don't support server-driven, multi-selection, iteratorTrackBy index. - - function testTrackBy(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - itemsInstance.all = itemsA; - tick(); - selectionInstance.currentSingle = itemsA[0]; - testSelection(true, false, false); - itemsInstance.all = itemsB; - tick(); - testSelection(true, false, false); - // itemsInstance.all = itemsC; - // tick(); - // testSelection(false, false, true); - // expect(selectionInstance.currentSingle.modified).toEqual(true); - }); - } - - it( - 'should support iteratorTrackBy item id', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; - }) - ); - - it( - 'should support iteratorTrackBy index', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = index => index; - }) - ); - - it( - 'should support datagridTrackBy item id', - testTrackBy(() => { - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it( - 'should use datagridTrackBy instead of iteratorTrackBy', - testTrackBy(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item.id; - }) - ); - - it('accepts pre-selected items with iteratorTrackBy when `all` has not been defined', fakeAsync(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item.id; + + it('should support trackBy item id', fakeAsync(() => { + itemsInstance.trackBy = item => item.id; + itemsInstance.all = itemsA; + tick(); + selectionInstance.currentSingle = itemsA[0]; + testSelection(true, false, false); + itemsInstance.all = itemsB; + tick(); + testSelection(true, false, false); + // itemsInstance.all = itemsC; + // tick(); + // testSelection(false, false, true); + // expect(selectionInstance.currentSingle.modified).toEqual(true); + })); + + it('accepts pre-selected items with trackBy when `all` has not been defined', fakeAsync(() => { + itemsInstance.trackBy = item => item.id; selectionInstance.currentSingle = { id: 1 }; tick(); itemsInstance.all = itemsA; @@ -960,17 +736,6 @@ export default function (): void { expect(selectionInstance.isSelected(4)).toBe(false); }); - it('should not execute canItBeLocked block when there are no items to scan inside lockItem and isLocked method', function () { - itemsInstance.all = undefined; - selectionInstance = new Selection(itemsInstance, filtersInstance); - selectionInstance.selectionType = SelectionType.Multi; - - // lock a row - selectionInstance.lockItem(4, true); - // make sure it's locked - expect(selectionInstance.isLocked(4)).toBe(false); - }); - /* * single selection is done with radio button so there is no logic to prevent * it from the provider - this must be handle at the component level @@ -1024,78 +789,23 @@ export default function (): void { expect(selectionInstance.isLocked(6)).toBe(false); }); - function testTrackByInteger(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - selectionInstance.selectionType = SelectionType.Multi; - selectionInstance.lockItem(2, true); - // Create new list of items - itemsInstance.all = [2, 5, 6, 7]; - expect(selectionInstance.isLocked(2)).toBe(true); - }); - } - - it( - 'should use iteratorTrackBy to find already locked items when the list is replaced (integer)', - testTrackByInteger(() => { - itemsInstance.iteratorTrackBy = (_index, item) => item; - }) - ); - - it( - 'should use datagridTrackBy to find already locked items when the list is replaced (integer)', - testTrackByInteger(() => { - itemsInstance.datagridTrackBy = item => item; - }) - ); - - it( - 'should use datagridTrackBy (and not iteratorTrackBy) to find already locked items when the list is replaced (integer)', - testTrackByInteger(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); - - itemsInstance.datagridTrackBy = item => item; - }) - ); - - function testTrackByObject(setTrackBy: () => void) { - return fakeAsync(function () { - setTrackBy(); - selectionInstance.selectionType = SelectionType.Multi; - itemsInstance.all = [{ id: 1 }, { id: 2 }]; - selectionInstance.lockItem({ id: 2 }, true); - // Create new list of items - itemsInstance.all = [{ id: 1 }, { id: 2 }]; - expect(selectionInstance.isLocked({ id: 2 })).toBe(true); - }); - } - - it( - 'should use iteratorTrackBy to find already locked items when the list is replaced (object)', - testTrackByObject(() => { - itemsInstance.iteratorTrackBy = (_index, { id }) => id; - }) - ); - - it( - 'should use datagridTrackBy to find already locked items when the list is replaced (object)', - testTrackByObject(() => { - itemsInstance.datagridTrackBy = ({ id }) => id; - }) - ); - - it( - 'should use datagridTrackBy (and not iteratorTrackBy) to find already locked items when the list is replaced (object)', - testTrackByObject(() => { - itemsInstance.iteratorTrackBy = jasmine - .createSpy('iteratorTrackBy') - .and.throwError('iteratorTrackBy should not be called when datagridTrackBy is provided'); + it('should use trackBy to find already locked items when the list is replaced (integer)', function () { + selectionInstance.selectionType = SelectionType.Multi; + selectionInstance.lockItem(2, true); + // Create new list of items + itemsInstance.all = [2, 5, 6, 7]; + expect(selectionInstance.isLocked(2)).toBe(true); + }); - itemsInstance.datagridTrackBy = ({ id }) => id; - }) - ); + it('should use trackBy to find already locked items when the list is replaced (object)', function () { + itemsInstance.trackBy = ({ id }) => id; + selectionInstance.selectionType = SelectionType.Multi; + itemsInstance.all = [{ id: 1 }, { id: 2 }]; + selectionInstance.lockItem({ id: 2 }, true); + // Create new list of items + itemsInstance.all = [{ id: 1 }, { id: 2 }]; + expect(selectionInstance.isLocked({ id: 2 })).toBe(true); + }); }); }); } diff --git a/projects/angular/src/data/datagrid/providers/selection.ts b/projects/angular/src/data/datagrid/providers/selection.ts index f42875c3b1..3c8ac36b1e 100644 --- a/projects/angular/src/data/datagrid/providers/selection.ts +++ b/projects/angular/src/data/datagrid/providers/selection.ts @@ -86,13 +86,11 @@ export class Selection { // if the currentSingle has been set before data was loaded, we look up and save the ref from current data set if (this.currentSingle && !this.prevSingleSelectionRef) { - if (this._items.canTrackBy()) { - this.prevSingleSelectionRef = this._items.trackBy(this.currentSingle); - } + this.prevSingleSelectionRef = this._items.trackBy(this.currentSingle); } - updatedItems.forEach((item, index) => { - const ref = this._items.trackBy(item, index); + updatedItems.forEach(item => { + const ref = this._items.trackBy(item); // If one of the updated items is the previously selectedSingle, set it as the new one if (this.prevSingleSelectionRef === ref) { newSingle = item; @@ -129,12 +127,10 @@ export class Selection { // if the current has been set before data was loaded, we look up and save the ref from current data set if (this.current.length > 0 && this.prevSelectionRefs.length !== this.current.length) { - if (this._items.canTrackBy()) { - this.prevSelectionRefs = []; - this.current.forEach(item => { - this.prevSelectionRefs.push(this._items.trackBy(item)); - }); - } + this.prevSelectionRefs = []; + this.current.forEach(item => { + this.prevSelectionRefs.push(this._items.trackBy(item)); + }); } // Duplicate loop, when the issue is issue#2342 is revisited keep in mind that @@ -142,8 +138,8 @@ export class Selection { // locked or not and update it. When only add items that are found in the lockedRefs back. // // The both loops below that goes over updatedItems could be combined into one. - updatedItems.forEach((item, index) => { - const ref = this._items.trackBy(item, index); + updatedItems.forEach(item => { + const ref = this._items.trackBy(item); if (this.lockedRefs.indexOf(ref) > -1) { updateLockedRef.push(ref); } @@ -153,8 +149,8 @@ export class Selection { // currently, the selection is cleared when filter is applied, so the logic inside // the if statement below results in broken behavior. if (leftOver.length > 0) { - updatedItems.forEach((item, index) => { - const ref = this._items.trackBy(item, index); + updatedItems.forEach(item => { + const ref = this._items.trackBy(item); // Look in current selected refs array if item is selected, and update actual value const selectedIndex = this.prevSelectionRefs.indexOf(ref); if (selectedIndex > -1) { @@ -228,7 +224,7 @@ export class Selection { } this._currentSingle = value; - if (this._items.canTrackBy() && value) { + if (value) { this.prevSingleSelectionRef = this._items.trackBy(value); } this.emitChange(); @@ -386,10 +382,8 @@ export class Selection { */ private selectItem(item: T): void { this.current = this.current.concat(item); - if (this._items.canTrackBy()) { - // Push selected ref onto array - this.prevSelectionRefs.push(this._items.trackBy(item)); - } + // Push selected ref onto array + this.prevSelectionRefs.push(this._items.trackBy(item)); } /** @@ -409,7 +403,7 @@ export class Selection { * Make sure that it could be locked */ private canItBeLocked(): boolean { - return this._selectionType !== SelectionType.None && this._items.canTrackBy(); + return this._selectionType !== SelectionType.None; } private emitChange() {