From 1f186e11266a506c440dd2ab7a1251ce4923559b Mon Sep 17 00:00:00 2001 From: gerhardboer Date: Fri, 17 Feb 2017 22:05:00 +0100 Subject: [PATCH] Fix for unsubscribing from the wrong column when a column has been toggled. --- src/directives/orderable.directive.spec.ts | 78 ++++++++++++++++++++-- src/directives/orderable.directive.ts | 40 ++++++----- 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/src/directives/orderable.directive.spec.ts b/src/directives/orderable.directive.spec.ts index 3b9401b9a..e170a3028 100644 --- a/src/directives/orderable.directive.spec.ts +++ b/src/directives/orderable.directive.spec.ts @@ -1,8 +1,10 @@ -import { async, TestBed, ComponentFixture } from '@angular/core/testing'; -import { Component, DebugElement } from '@angular/core'; -import { By } from '@angular/platform-browser'; +import {async, ComponentFixture, TestBed} from "@angular/core/testing"; +import {Component, ElementRef} from "@angular/core"; +import {By} from "@angular/platform-browser"; -import { OrderableDirective } from '.'; +import {OrderableDirective} from "."; +import {DraggableDirective} from "./draggable.directive"; +import {id} from "../utils/id"; @Component({ selector: 'test-fixture-component', @@ -21,7 +23,7 @@ describe('OrderableDirective', () => { // provide our implementations or mocks to the dependency injector beforeEach(() => { TestBed.configureTestingModule({ - declarations: [ + declarations: [ OrderableDirective, TestFixtureComponent ] @@ -45,7 +47,7 @@ describe('OrderableDirective', () => { describe('fixture', () => { let directive: OrderableDirective; - + beforeEach(() => { directive = fixture.debugElement .query(By.directive(OrderableDirective)) @@ -59,5 +61,69 @@ describe('OrderableDirective', () => { it('should have OrderableDirective directive', () => { expect(directive).toBeTruthy(); }); + + describe('when a draggable is removed', () => { + + function checkAllSubscriptionsForActiveObservers() { + let subs = directive.draggables.map(d => { + expect(d.dragEnd.isStopped).toBe(false); + expect(d.dragStart.isStopped).toBe(false); + + return { + dragStart: d.dragStart.observers, + dragEnd: d.dragEnd.observers + } + }); + + subs.forEach((sub) => { + expect(sub.dragStart.length).toBe(1); + expect(sub.dragEnd.length).toBe(1); + + }) + } + + function newDraggable() { + const draggable = new DraggableDirective({}); + + // used for the KeyValueDiffer + draggable.dragModel = { + $$id: id() + }; + + return draggable; + } + + let draggables; + + beforeEach(() => { + draggables = [ + newDraggable(), + newDraggable(), + newDraggable() + ]; + + directive.draggables.reset(draggables); + + directive.updateSubscriptions(); + + checkAllSubscriptionsForActiveObservers(); + }); + + it('then dragStart and dragEnd are unsubscribed from the removed draggable', () => { + let unsubbed = draggables.splice(0,1)[0]; + + expect(unsubbed.dragStart.isStopped).toBe(false); + expect(unsubbed.dragEnd.isStopped).toBe(false); + + directive.draggables.reset(draggables); + + directive.updateSubscriptions(); + + checkAllSubscriptionsForActiveObservers(); + + expect(unsubbed.dragStart.isStopped).toBe(true); + expect(unsubbed.dragEnd.isStopped).toBe(true); + }); + }); }); }); diff --git a/src/directives/orderable.directive.ts b/src/directives/orderable.directive.ts index 8458590a6..b4cd2f8ce 100644 --- a/src/directives/orderable.directive.ts +++ b/src/directives/orderable.directive.ts @@ -3,14 +3,14 @@ import { QueryList, KeyValueDiffers, AfterContentInit, OnDestroy } from '@angular/core'; -import { DraggableDirective } from './draggable.directive'; +import {DraggableDirective} from './draggable.directive'; -@Directive({ selector: '[orderable]' }) +@Directive({selector: '[orderable]'}) export class OrderableDirective implements AfterContentInit, OnDestroy { @Output() reorder: EventEmitter = new EventEmitter(); - @ContentChildren(DraggableDirective, { descendants: true }) + @ContentChildren(DraggableDirective, {descendants: true}) draggables: QueryList; positions: any; @@ -24,7 +24,7 @@ export class OrderableDirective implements AfterContentInit, OnDestroy { // HACK: Investigate Better Way this.updateSubscriptions(); this.draggables.changes.subscribe( - this.updateSubscriptions.bind(this)); + this.updateSubscriptions.bind(this)); } ngOnDestroy(): void { @@ -35,20 +35,20 @@ export class OrderableDirective implements AfterContentInit, OnDestroy { } updateSubscriptions(): void { - const diffs = this.differ.diff(this.draggables.toArray()); + const diffs = this.differ.diff(this.createMapDiffs()); - if(diffs) { - const subscribe = ({ currentValue, previousValue }: any) => { - unsubscribe({ previousValue }); + if (diffs) { + const subscribe = ({currentValue, previousValue}: any) => { + unsubscribe({previousValue}); - if(currentValue) { + if (currentValue) { currentValue.dragStart.subscribe(this.onDragStart.bind(this)); currentValue.dragEnd.subscribe(this.onDragEnd.bind(this)); } }; - const unsubscribe = ({ previousValue }: any) => { - if(previousValue) { + const unsubscribe = ({previousValue}: any) => { + if (previousValue) { previousValue.dragStart.unsubscribe(); previousValue.dragEnd.unsubscribe(); } @@ -60,31 +60,39 @@ export class OrderableDirective implements AfterContentInit, OnDestroy { } } + private createMapDiffs(): { [key: string]: DraggableDirective } { + return this.draggables.toArray() + .reduce((acc, curr) => { + acc[curr.dragModel.$$id] = curr; + return acc; + }, {}); + } + onDragStart(): void { this.positions = {}; let i = 0; - for(let dragger of this.draggables.toArray()) { + for (let dragger of this.draggables.toArray()) { let elm = dragger.element; - this.positions[dragger.dragModel.prop] = { + this.positions[dragger.dragModel.prop] = { left: parseInt(elm.offsetLeft.toString(), 0), index: i++ }; } } - onDragEnd({ element, model }: any) { + onDragEnd({element, model}: any) { const newPos = parseInt(element.offsetLeft.toString(), 0); const prevPos = this.positions[model.prop]; let i = 0; - for(let prop in this.positions) { + for (let prop in this.positions) { let pos = this.positions[prop]; let movedLeft = newPos < pos.left && prevPos.left > pos.left; let movedRight = newPos > pos.left && prevPos.left < pos.left; - if(movedLeft || movedRight) { + if (movedLeft || movedRight) { this.reorder.emit({ prevIndex: prevPos.index, newIndex: i,