[ng] Clean up datagrid selection preservation (#2038)

Signed-off-by: Jeeyun Lim <[email protected]>

src/app/datagrid/full/full.html

@@ -77,7 +77,7 @@ <h2>Full Datagrid demo</h2>
         <clr-dg-column class="lorem-ipsum" *ngIf="loremIpsumColumn">Multi-line text</clr-dg-column>
 
         <clr-dg-placeholder>No users found</clr-dg-placeholder>
-        <clr-dg-row *clrDgItems="let user of users" [clrDgItem]="user">
+        <clr-dg-row *clrDgItems="let user of users; trackBy: trackById" [clrDgItem]="user">
             <clr-dg-cell>{{user.id}}</clr-dg-cell>
             <clr-dg-cell>{{user.name}}</clr-dg-cell>
             <clr-dg-cell>{{user.creation | date}}</clr-dg-cell>
@@ -115,7 +115,7 @@ <h2>Full Datagrid demo</h2>
         <clr-dg-column class="lorem-ipsum" *ngIf="loremIpsumColumn">Multi-line text</clr-dg-column>
 
         <clr-dg-placeholder>No users found</clr-dg-placeholder>
-        <clr-dg-row *ngFor="let user of users" [clrDgItem]="user">
+        <clr-dg-row *ngFor="let user of users; trackBy: trackById" [clrDgItem]="user">
             <clr-dg-cell>{{user.id}}</clr-dg-cell>
             <clr-dg-cell>{{user.name}}</clr-dg-cell>
             <clr-dg-cell>{{user.creation | date}}</clr-dg-cell>

src/app/datagrid/full/full.ts

@@ -11,12 +11,11 @@ import {ColorFilter} from "../utils/color-filter";
 import {PokemonComparator} from "../utils/pokemon-comparator";
 import {PokemonFilter} from "../utils/pokemon-filter";
 
-
 @Component({
     selector: "clr-datagrid-full-demo",
     providers: [Inventory],
     templateUrl: "./full.html",
-    styleUrls: ["../datagrid.demo.scss"]
+    styleUrls: ["../datagrid.demo.scss"],
 })
 export class DatagridFullDemo {
     options = {
@@ -82,18 +81,29 @@ export class DatagridFullDemo {
                 if (filter instanceof ColorFilter) {
                     filters.color = (<ColorFilter>filter).listSelected();
                 } else {
-                    const {property, value} = <{property: string, value: string}>filter;
+                    const {property, value} = < {
+                        property: string;
+                        value: string
+                    }
+                    > filter;
                     filters[property] = [value];
                 }
             }
         }
         this.inventory.filter(filters)
-            .sort(<{by: string, reverse: boolean}>state.sort)
+            .sort(<{
+                      by: string;
+                      reverse: boolean
+                  }>state.sort)
             .fetch(state.page && state.page.from, state.page && state.page.size)
             .then((result: FetchResult) => {
                 this.users = result.users;
                 this.total = result.length;
                 this.loading = false;
             });
     }
+
+    trackById(idx, item) {
+        return item.id;
+    }
 }

src/clr-angular/data/datagrid/datagrid.spec.ts

@@ -36,7 +36,7 @@ export default function(): void {
             it("allows to manually force a refresh of displayed items when data mutates", function() {
                 const items: Items = context.getClarityProvider(Items);
                 let refreshed = false;
-                items.change.subscribe(() => refreshed = true);
+                items.change.subscribe(() => (refreshed = true));
                 expect(refreshed).toBe(false);
                 context.clarityDirective.dataChanged();
                 expect(refreshed).toBe(true);
@@ -155,7 +155,7 @@ export default function(): void {
                             by: comparator,
                             reverse: false,
                         },
-                        filters: [filter]
+                        filters: [filter],
                     });
                 });
 
@@ -171,7 +171,9 @@ export default function(): void {
                     filters.add(builtinStringFilter);
                     context.detectChanges();
                     expect(context.testComponent.latestState.filters).toEqual([
-                        customFilter, testStringFilter, {property: "test", value: "1234"}
+                        customFilter,
+                        testStringFilter,
+                        {property: "test", value: "1234"},
                     ]);
                 });
 
@@ -299,7 +301,7 @@ export default function(): void {
 
             describe("Template API", function() {
                 it("sets the currentSingle binding", function() {
-                    expect(selection.currentSingle).toBeNull();
+                    expect(selection.currentSingle).toBeUndefined();
                     context.testComponent.selected = 1;
                     context.detectChanges();
                     expect(selection.currentSingle).toEqual(1);
@@ -308,8 +310,68 @@ export default function(): void {
                     expect(selection.currentSingle).toBeNull();
                 });
 
+                it("does not emit a change event for on initialization, before selection", function() {
+                    let singleSelectedchangeCount: number = 0;
+                    const sub =
+                        context.clarityDirective.singleSelectedChanged.subscribe(s => singleSelectedchangeCount++);
+
+                    expect(selection.currentSingle).toBeUndefined();
+                    expect(singleSelectedchangeCount).toEqual(0);
+
+                    sub.unsubscribe();
+                });
+
+                it("it emits a change event when changing the selection", function() {
+                    let singleSelectedchangeCount: number = 0;
+                    const sub =
+                        context.clarityDirective.singleSelectedChanged.subscribe(s => singleSelectedchangeCount++);
+
+                    context.testComponent.selected = 1;
+                    context.detectChanges();
+                    expect(selection.currentSingle).toEqual(1);
+                    expect(singleSelectedchangeCount).toEqual(1);
+
+                    sub.unsubscribe();
+                });
+
+                it("it does not emit a change event when setting selection to undefined/null if already undefined/null",
+                   function() {
+                       let singleSelectedchangeCount: number = 0;
+                       const sub =
+                           context.clarityDirective.singleSelectedChanged.subscribe(s => singleSelectedchangeCount++);
+
+                       expect(selection.currentSingle).toBeUndefined();
+                       expect(singleSelectedchangeCount).toEqual(0);
+
+                       context.testComponent.selected = null;
+                       context.detectChanges();
+                       expect(selection.currentSingle).toBeUndefined();
+                       expect(singleSelectedchangeCount).toEqual(0);
+
+                       sub.unsubscribe();
+                   });
+
+                it("it does not emit a change event when selecting the same value", function() {
+                    let singleSelectedchangeCount: number = 0;
+                    const sub =
+                        context.clarityDirective.singleSelectedChanged.subscribe(s => singleSelectedchangeCount++);
+
+                    context.testComponent.selected = 1;
+                    context.detectChanges();
+                    expect(selection.currentSingle).toEqual(1);
+                    expect(singleSelectedchangeCount).toEqual(1);
+
+                    // re-assigning to the same value should not increase the singleSelectedchangeCount
+                    context.testComponent.selected = 1;
+                    context.detectChanges();
+                    expect(selection.currentSingle).toEqual(1);
+                    expect(singleSelectedchangeCount).toEqual(1);
+
+                    sub.unsubscribe();
+                });
+
                 it("offers two way binding on the currentSingle value", function() {
-                    expect(selection.currentSingle).toBeNull();
+                    expect(selection.currentSingle).toBeUndefined();
                     context.testComponent.selected = 1;
                     context.detectChanges();
                     expect(selection.currentSingle).toEqual(1);
@@ -417,7 +479,7 @@ export default function(): void {
 
         <clr-dg-footer>{{items.length}} items</clr-dg-footer>
     </clr-datagrid>
-`
+`,
 })
 class FullTest {
     items = [1, 2, 3];
@@ -456,7 +518,7 @@ class FullTest {
 
         <clr-dg-footer>{{items.length}} items</clr-dg-footer>
     </clr-datagrid>
-`
+`,
 })
 class NgForTest {
     items = [1, 2, 3];
@@ -475,7 +537,7 @@ class NgForTest {
 
         <clr-dg-footer>{{items.length}} items</clr-dg-footer>
     </clr-datagrid>
-`
+`,
 })
 class TrackByTest {
     items = [1, 2, 3];
@@ -490,7 +552,7 @@ class TrackByTest {
 @Component({
     template: `
     <multi-select-test [items]="items" [selected]="selected"></multi-select-test>
-    `
+    `,
 })
 class OnPushTest {
     items = [1, 2, 3];
@@ -509,7 +571,7 @@ class OnPushTest {
             <clr-dg-cell>{{item * item}}</clr-dg-cell>
         </clr-dg-row>
     </clr-datagrid>`,
-    changeDetection: ChangeDetectionStrategy.OnPush
+    changeDetection: ChangeDetectionStrategy.OnPush,
 })
 class MultiSelectionTest {
     @Input() items: any[] = [];
@@ -529,7 +591,7 @@ class MultiSelectionTest {
 
         <clr-dg-footer (click)="selected = null">{{selected}}</clr-dg-footer>
     </clr-datagrid>
-`
+`,
 })
 class SingleSelectionTest {
     items = [1, 2, 3];
@@ -554,7 +616,7 @@ class SingleSelectionTest {
 
         <clr-dg-footer>{{items.length}} items</clr-dg-footer>
     </clr-datagrid>
-`
+`,
 })
 class ActionableRowTest {
     items = [1, 2, 3];
@@ -577,14 +639,13 @@ class ActionableRowTest {
 
         <clr-dg-footer>{{items.length}} items</clr-dg-footer>
     </clr-datagrid>
-`
+`,
 })
 class ExpandableRowTest {
     items = [1, 2, 3];
     expandable = true;
 }
 
-
 @Component({
     template: `
         <clr-datagrid>
@@ -604,7 +665,7 @@ class ExpandableRowTest {
 
             <clr-dg-footer>{{items.length}} items</clr-dg-footer>
         </clr-datagrid>
-    `
+    `,
 })
 class ChocolateClrDgItemsTest {
     items = [1, 2, 3];
@@ -631,7 +692,7 @@ class ChocolateClrDgItemsTest {
 
             <clr-dg-footer>{{items.length}} items</clr-dg-footer>
         </clr-datagrid>
-    `
+    `,
 })
 class ChocolateNgForTest {
     items = [1, 2, 3];
@@ -663,7 +724,6 @@ class TestStringFilter implements ClrDatagridStringFilterInterface<number> {
     }
 }
 
-
 @Component({
     selector: "hidden-column-test",
     template: `
@@ -679,7 +739,7 @@ class TestStringFilter implements ClrDatagridStringFilterInterface<number> {
             <clr-dg-cell>{{item}}</clr-dg-cell>
             <clr-dg-cell>{{item * item}}</clr-dg-cell>
         </clr-dg-row>
-    </clr-datagrid>`
+    </clr-datagrid>`,
 })
 class HiddenColumnTest {
     items = [1, 2, 3];

src/clr-angular/data/datagrid/datagrid.ts

@@ -13,7 +13,7 @@ import {
     Input,
     OnDestroy,
     Output,
-    QueryList
+    QueryList,
 } from "@angular/core";
 import {Subscription} from "rxjs/Subscription";
 
@@ -52,7 +52,7 @@ import {DatagridRenderOrganizer} from "./render/render-organizer";
         StateProvider,
         ColumnToggleButtonsService,
     ],
-    host: {"[class.datagrid-host]": "true"}
+    host: {"[class.datagrid-host]": "true"},
 })
 export class ClrDatagrid implements AfterContentInit, AfterViewInit, OnDestroy {
     constructor(private columnService: HideableColumnService, private organizer: DatagridRenderOrganizer,
@@ -112,9 +112,12 @@ export class ClrDatagrid implements AfterContentInit, AfterViewInit, OnDestroy {
     @Input("clrDgSingleSelected")
     set singleSelected(value: any) {
         this.selection.selectionType = SelectionType.Single;
+        // the clrDgSingleSelected is updated in one of two cases:
+        // 1. an explicit value is passed
+        // 2. is being set to null or undefined, where previously it had a value
         if (value) {
             this.selection.currentSingle = value;
-        } else {
+        } else if (this.selection.currentSingle) {
             this.selection.currentSingle = null;
         }
     }

src/clr-angular/data/datagrid/providers/selection.spec.ts

@@ -250,7 +250,18 @@ export default function(): void {
         });
 
         describe("client-side selection and pagination", function() {
-            const items = [{id: 1}, {id: 2}, {id: 3}, {id: 4}, {id: 5}, {id: 6}, {id: 7}, {id: 8}, {id: 9}, {id: 10}];
+            const items = [
+                {id: 1},
+                {id: 2},
+                {id: 3},
+                {id: 4},
+                {id: 5},
+                {id: 6},
+                {id: 7},
+                {id: 8},
+                {id: 9},
+                {id: 10},
+            ];
 
             function cloneItems() {
                 return items.map(item => {
@@ -313,6 +324,16 @@ export default function(): void {
                     expect(selectionInstance.isSelected(items[5])).toBe(true);
                 });
 
+                it("should clear selection if it is no longer in dataset", fakeAsync(() => {
+                       selectionInstance.currentSingle = items[2];
+                       pageInstance.current = 2;
+                       expect(selectionInstance.isSelected(items[2])).toBe(true);
+
+                       itemsInstance.all = cloneItems().splice(2, 1);
+                       tick();
+                       expect(selectionInstance.currentSingle).toBe(undefined);
+                   }));
+
                 it("does not apply trackBy to single selection with no items", () => {
                     const emptyItems = new Items(filtersInstance, sortInstance, pageInstance);
                     const selection: Selection = new Selection(emptyItems, filtersInstance);