From d26735862ed9e7f49606281cc2bda9b5b0db7065 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Wed, 17 Jul 2019 15:24:09 -0400 Subject: [PATCH 01/14] added isActive class to repeater --- e2e/repeater.e2e-spec.ts | 18 ++++++------- .../fixtures/repeater.component.fixture.html | 1 + .../fixtures/repeater.component.fixture.ts | 15 ++++++----- .../repeater/repeater-item.component.html | 1 + .../repeater/repeater-item.component.scss | 7 ++++++ .../repeater/repeater-item.component.ts | 3 +++ .../repeater/repeater.component.spec.ts | 25 +++++++++++++++++++ .../repeater/repeater-visual.component.html | 23 +++++++++++++++++ .../repeater/repeater-visual.component.ts | 6 +++-- 9 files changed, 82 insertions(+), 17 deletions(-) diff --git a/e2e/repeater.e2e-spec.ts b/e2e/repeater.e2e-spec.ts index 0bcebc06..c40e60c9 100644 --- a/e2e/repeater.e2e-spec.ts +++ b/e2e/repeater.e2e-spec.ts @@ -4,15 +4,6 @@ import { } from '@skyux-sdk/e2e'; describe('Repeater', () => { - it('should match the baseline sort screenshot', (done) => { - SkyHostBrowser.get('visual/sort'); - SkyHostBrowser.setWindowBreakpoint('lg'); - SkyHostBrowser.scrollTo('#screenshot-sort-full'); - expect('#screenshot-sort-full').toMatchBaselineScreenshot(done, { - screenshotName: 'sort' - }); - }); - it('should match previous repeater screenshot', (done) => { SkyHostBrowser.get('visual/repeater'); SkyHostBrowser.setWindowBreakpoint('lg'); @@ -22,6 +13,15 @@ describe('Repeater', () => { }); }); + it('should match previous repeater screenshot when an item is active', (done) => { + SkyHostBrowser.get('visual/repeater'); + SkyHostBrowser.setWindowBreakpoint('lg'); + SkyHostBrowser.scrollTo('#screenshot-repeater-with-active-item'); + expect('#screenshot-repeater-with-active-item').toMatchBaselineScreenshot(done, { + screenshotName: 'repeater-with-active-item' + }); + }); + it('should match previous repeater screenshot when all are collapsed', (done) => { SkyHostBrowser.get('visual/repeater'); SkyHostBrowser.setWindowBreakpoint('lg'); diff --git a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html index cdf1749f..816ef9db 100644 --- a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html +++ b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html @@ -1,5 +1,6 @@ diff --git a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts index 33fc98f4..8349a83d 100644 --- a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts +++ b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts @@ -12,19 +12,22 @@ import { templateUrl: './repeater.component.fixture.html' }) export class RepeaterTestComponent { - @ViewChild(SkyRepeaterComponent) - public repeater: SkyRepeaterComponent; - - public showContextMenu: boolean; - - public removeLastItem: boolean; public expandMode = 'single'; + public isActive = false; + public lastItemExpanded: boolean; public lastItemSelected = false; + public removeLastItem: boolean; + + public showContextMenu: boolean; + + @ViewChild(SkyRepeaterComponent) + public repeater: SkyRepeaterComponent; + public onCollapse(): void {} public onExpand(): void {} diff --git a/src/app/public/modules/repeater/repeater-item.component.html b/src/app/public/modules/repeater/repeater-item.component.html index e5b6915b..3bae3cc3 100644 --- a/src/app/public/modules/repeater/repeater-item.component.html +++ b/src/app/public/modules/repeater/repeater-item.component.html @@ -1,6 +1,7 @@
{ })); }); + it('should add and remove active css class when isActive value changes', fakeAsync(() => { + let fixture = TestBed.createComponent(RepeaterTestComponent); + let cmp: RepeaterTestComponent = fixture.componentInstance; + let el = fixture.nativeElement; + fixture.detectChanges(); + tick(); + + let activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(0); + + cmp.isActive = true; + fixture.detectChanges(); + tick(); + + activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(1); + + cmp.isActive = false; + fixture.detectChanges(); + tick(); + + activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(0); + })); + describe('with inline-form', () => { let fixture: ComponentFixture; let el: HTMLElement; diff --git a/src/app/visual/repeater/repeater-visual.component.html b/src/app/visual/repeater/repeater-visual.component.html index c8014a66..4b20e074 100644 --- a/src/app/visual/repeater/repeater-visual.component.html +++ b/src/app/visual/repeater/repeater-visual.component.html @@ -20,6 +20,29 @@

Basic repeater

+
+

Repeater with isActive enabled on click

+ + + Title 1 + Content 1 + + + Title 2 + Content 2 + + +
+
Date: Wed, 17 Jul 2019 15:28:55 -0400 Subject: [PATCH 02/14] updated active class to use sass variables --- .../public/modules/repeater/repeater-item.component.scss | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.scss b/src/app/public/modules/repeater/repeater-item.component.scss index b82343e7..c39bbd19 100644 --- a/src/app/public/modules/repeater/repeater-item.component.scss +++ b/src/app/public/modules/repeater/repeater-item.component.scss @@ -23,10 +23,10 @@ } &.sky-repeater-item-active { - background-color: #eeeeef; - color: #212327; + background-color: $sky-background-color-neutral-light; + color: $sky-text-color-default; padding-left: 6px; - border-left: 4px solid #00b4f1; + border-left: $sky-nav-selected-border-width solid $sky-highlight-color-info; } } From 954bd2b1ff71d21c1c46abc3ad0084c3895602aa Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Wed, 17 Jul 2019 15:41:28 -0400 Subject: [PATCH 03/14] style updates --- .../repeater/repeater-item.component.ts | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.ts b/src/app/public/modules/repeater/repeater-item.component.ts index b1032754..ef69f3f3 100644 --- a/src/app/public/modules/repeater/repeater-item.component.ts +++ b/src/app/public/modules/repeater/repeater-item.component.ts @@ -38,19 +38,23 @@ let nextId: number = 0; animations: [skyAnimationSlide] }) export class SkyRepeaterItemComponent implements OnDestroy { - public contentId: string = `sky-radio-content-${++nextId}`; - public get isExpanded(): boolean { - return this._isExpanded; - } + @Input() + public inlineFormConfig: SkyInlineFormConfig; + + @Input() + public inlineFormTemplate: TemplateRef; + + @Input() + public isActive: boolean = false; @Input() public set isExpanded(value: boolean) { this.updateForExpanded(value, true); } - public get isSelected(): boolean { - return this._isSelected; + public get isExpanded(): boolean { + return this._isExpanded; } @Input() @@ -58,23 +62,15 @@ export class SkyRepeaterItemComponent implements OnDestroy { this._isSelected = value; } - @Input() - public isActive: boolean = false; - - @Input() - public showInlineForm: boolean = false; - - @Input() - public inlineFormConfig: SkyInlineFormConfig; + public get isSelected(): boolean { + return this._isSelected; + } @Input() - public inlineFormTemplate: TemplateRef; - - @Output() - public inlineFormClose = new EventEmitter(); + public selectable: boolean = false; @Input() - public selectable: boolean = false; + public showInlineForm: boolean = false; @Output() public collapse = new EventEmitter(); @@ -82,11 +78,11 @@ export class SkyRepeaterItemComponent implements OnDestroy { @Output() public expand = new EventEmitter(); - public slideDirection: string; + @Output() + public inlineFormClose = new EventEmitter(); + + public contentId: string = `sky-radio-content-${++nextId}`; - public get isCollapsible(): boolean { - return this._isCollapsible; - } public set isCollapsible(value: boolean) { if (this.isCollapsible !== value) { this._isCollapsible = value; @@ -98,6 +94,12 @@ export class SkyRepeaterItemComponent implements OnDestroy { } } + public get isCollapsible(): boolean { + return this._isCollapsible; + } + + public slideDirection: string; + private _isCollapsible = true; private _isExpanded = true; From f7a6fb9af37e1976fd79c1107fc84c57bb7159d3 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Mon, 22 Jul 2019 13:19:28 -0400 Subject: [PATCH 04/14] added service with activeIndex emitter --- .../repeater/repeater-item.component.html | 2 +- .../repeater/repeater-item.component.ts | 31 ++++++- .../modules/repeater/repeater.component.ts | 63 ++++++++++++- .../modules/repeater/repeater.service.ts | 91 ++++++++++++++++++- .../repeater/repeater-visual.component.html | 44 +++++---- .../repeater/repeater-visual.component.ts | 36 +++++++- 6 files changed, 235 insertions(+), 32 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.html b/src/app/public/modules/repeater/repeater-item.component.html index 3bae3cc3..b68039fd 100644 --- a/src/app/public/modules/repeater/repeater-item.component.html +++ b/src/app/public/modules/repeater/repeater-item.component.html @@ -1,7 +1,7 @@
; - @Input() - public isActive: boolean = false; - @Input() public set isExpanded(value: boolean) { this.updateForExpanded(value, true); @@ -81,7 +80,9 @@ export class SkyRepeaterItemComponent implements OnDestroy { @Output() public inlineFormClose = new EventEmitter(); - public contentId: string = `sky-radio-content-${++nextId}`; + public active: boolean = false; + + public contentId: string = `sky-radio-content-${++nextContentId}`; public set isCollapsible(value: boolean) { if (this.isCollapsible !== value) { @@ -98,6 +99,8 @@ export class SkyRepeaterItemComponent implements OnDestroy { return this._isCollapsible; } + public itemId: string = `sky-repeater-item-${++nextItemId}`; + public slideDirection: string; private _isCollapsible = true; @@ -118,6 +121,7 @@ export class SkyRepeaterItemComponent implements OnDestroy { this.collapse.complete(); this.expand.complete(); this.inlineFormClose.complete(); + this.repeaterService.destroyItem(this); } public headerClick() { @@ -126,6 +130,23 @@ export class SkyRepeaterItemComponent implements OnDestroy { } } + public initializeItemIndex(): void { + setTimeout(() => { + this.repeaterService.addItem(this); + // TODO??? + // this.repeaterService.activeItemIndex.subscribe((id: any) => { + // if (id) { + // this.active = this.itemId === id; + // } + // this.changeDetector.markForCheck(); + // }); + + // if (this.active) { + // this.repeaterService.activateItem(this); + // } + }); + } + public chevronDirectionChange(direction: string) { this.updateForExpanded(direction === 'up', true); } diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index 8dbbc611..4955de36 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -3,9 +3,14 @@ import { Component, ContentChildren, Input, - QueryList + QueryList, + OnChanges, + OnDestroy, + SimpleChanges } from '@angular/core'; +import 'rxjs/add/operator/distinctUntilChanged'; + import { SkyRepeaterItemComponent } from './repeater-item.component'; @@ -17,9 +22,14 @@ import { @Component({ selector: 'sky-repeater', styleUrls: ['./repeater.component.scss'], - templateUrl: './repeater.component.html' + templateUrl: './repeater.component.html', + providers: [SkyRepeaterService] }) -export class SkyRepeaterComponent implements AfterContentInit { +export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDestroy { + + @Input() + public activeIndex: number; + @Input() public set expandMode(value: string) { this._expandMode = value; @@ -35,7 +45,9 @@ export class SkyRepeaterComponent implements AfterContentInit { private _expandMode = 'none'; - constructor(private repeaterService: SkyRepeaterService) { + constructor( + private repeaterService: SkyRepeaterService + ) { this.repeaterService.itemCollapseStateChange.subscribe((item: SkyRepeaterItemComponent) => { if (this.expandMode === 'single' && item.isExpanded) { this.items.forEach((otherItem) => { @@ -50,6 +62,39 @@ export class SkyRepeaterComponent implements AfterContentInit { } public ngAfterContentInit() { + // initialize each item's index (in case items are instantiated out of order). + this.items.forEach(item => item.initializeItemIndex()); + this.items.changes.subscribe((change: QueryList) => { + this.repeaterService.items + .take(1) + .subscribe(currentItems => { + change + .filter(item => currentItems.indexOf(item) < 0) + .forEach(item => item.initializeItemIndex()); + }); + }); + + // If activeIndex has been set, call service to activate the appropriate item. + if (this.activeIndex || this.activeIndex === 0) { + this.repeaterService.activateItemByIndex(this.activeIndex); + } + + // Watch for changes to the activeIndex and activate the appropriate item. + this.repeaterService.activeItemIndex + .distinctUntilChanged() + .subscribe((index) => { + // HACK: Not selecting the active item in a timeout causes an error. + // https://github.com/angular/angular/issues/6005 + setTimeout(() => { + if (index !== this.activeIndex) { + this.activeIndex = index; + } + this.items.forEach((item, i) => { + item.active = i === index ? true : false; + }); + }); + }); + // HACK: Not updating for expand mode in a timeout causes an error. // https://github.com/angular/angular/issues/6005 this.items.changes.subscribe(() => { @@ -63,6 +108,16 @@ export class SkyRepeaterComponent implements AfterContentInit { }, 0); } + public ngOnChanges(changes: SimpleChanges): void { + if (changes['activeIndex'] && changes['activeIndex'].currentValue !== changes['activeIndex'].previousValue) { + this.repeaterService.activateItemByIndex(this.activeIndex); + } + } + + public ngOnDestroy(): void { + this.repeaterService.destroy(); + } + private updateForExpandMode(itemAdded?: SkyRepeaterItemComponent) { if (this.items) { let foundExpanded = false; diff --git a/src/app/public/modules/repeater/repeater.service.ts b/src/app/public/modules/repeater/repeater.service.ts index e8e37cfd..0995b5e6 100644 --- a/src/app/public/modules/repeater/repeater.service.ts +++ b/src/app/public/modules/repeater/repeater.service.ts @@ -3,15 +3,104 @@ import { Injectable } from '@angular/core'; +import { + BehaviorSubject +} from 'rxjs/BehaviorSubject'; + import { SkyRepeaterItemComponent } from './repeater-item.component'; @Injectable() export class SkyRepeaterService { + + public activeItemIndex: BehaviorSubject = new BehaviorSubject(undefined); + public itemCollapseStateChange = new EventEmitter(); - public onItemCollapseStateChange(item: SkyRepeaterItemComponent) { + public items: BehaviorSubject> = new BehaviorSubject>([]); + + public activateItem(item: SkyRepeaterItemComponent): void { + this.items.take(1).subscribe(items => { + const index = items.indexOf(item); + if (index > -1) { + this.activeItemIndex.next(index); + } + }); + } + + public activateItemByIndex(index: number): void { + this.items.take(1).subscribe(() => { + this.activeItemIndex.next(index); + }); + } + + public addItem(item: SkyRepeaterItemComponent): void { + this.items.take(1).subscribe((currentItems) => { + // let lastTabIndex = this.getLastItemIndex(currentItems); + // if (currentItems && (lastTabIndex || lastTabIndex === 0)) { + // item.itemId = lastTabIndex + 1; + // } + currentItems.push(item); + this.items.next(currentItems); + console.log('add item. new array:'); + console.log(currentItems); + }); + } + + public destroyItem(item: SkyRepeaterItemComponent): void { + this.items.take(1).subscribe((items) => { + const index = items.indexOf(item); + if (index > -1) { + if (item.active) { + // Try selecting the next tab first. + // If there's no next tab, try selecting the previous one. + let newActiveItem = items[index + 1] || items[index - 1]; + /*istanbul ignore else */ + if (newActiveItem) { + const newIndex = items.indexOf(newActiveItem); + this.activeItemIndex.next(newIndex); + } + } + items.splice(index, 1); + } + this.items.next(items); + }); + } + + public destroy(): void { + this.items.complete(); + this.activeItemIndex.complete(); + this.itemCollapseStateChange.complete(); + } + + public onItemCollapseStateChange(item: SkyRepeaterItemComponent): void { this.itemCollapseStateChange.emit(item); } + + private getLastItemIndex(tabs: Array): number { + // let result: any = undefined; + // for (let i = 0; i < tabs.length; i++) { + // if (typeof tabs[i].itemId === 'number' && + // (result === undefined || result < tabs[i].itemId)) { + // result = tabs[i].itemId; + // } + // } + return tabs.length; + } + + private getItemByIndex(index: number, items: Array) { + return items[index]; + // for (let i = 0, n = items.length; i < n; i++) { + // let item = items[i]; + // if (item.itemId === index) { + // return item; + // } + // } + // return undefined; + } + + private getIndexByItem(items: Array, item: SkyRepeaterItemComponent): number { + return items.indexOf(item); + } } diff --git a/src/app/visual/repeater/repeater-visual.component.html b/src/app/visual/repeater/repeater-visual.component.html index 4b20e074..954ae0d1 100644 --- a/src/app/visual/repeater/repeater-visual.component.html +++ b/src/app/visual/repeater/repeater-visual.component.html @@ -1,4 +1,4 @@ -
@@ -18,32 +18,42 @@

Basic repeater

Content 2 -
+
-->

Repeater with isActive enabled on click

- - - Title 1 - Content 1 - - + - Title 2 - Content 2 + + {{ item.title }} + + + {{ item.note }} + + +
-
@@ -263,4 +273,4 @@

Repeater with inline-form

-
+
--> diff --git a/src/app/visual/repeater/repeater-visual.component.ts b/src/app/visual/repeater/repeater-visual.component.ts index e36f35ce..5b2c351f 100644 --- a/src/app/visual/repeater/repeater-visual.component.ts +++ b/src/app/visual/repeater/repeater-visual.component.ts @@ -8,6 +8,8 @@ import { SkyInlineFormConfig } from '@skyux/inline-form'; +let nextItemId: number = 0; + @Component({ selector: 'repeater-visual', templateUrl: './repeater-visual.component.html', @@ -15,7 +17,7 @@ import { }) export class RepeaterVisualComponent { - public activeId = 1; + public activeIndex: number = 2; public activeInlineFormId: number; @@ -31,25 +33,51 @@ export class RepeaterVisualComponent { public items = [ { - id: '1', + id: 1, title: '2018 Gala', note: '2018 Gala for friends and family', fund: 'General 2018 Fund' }, { - id: '2', + id: 'abba', title: '2018 Special event', note: 'Special event', fund: '2018 Special Events Fund' }, { - id: '3', + id: 'dabba', + title: '2019 Gala', + note: '2019 Gala for friends and family', + fund: 'General 2019 Fund' + }, + { + id: 99, + title: '2019 Gala', + note: '2019 Gala for friends and family', + fund: 'General 2019 Fund' + }, + { + id: 123, title: '2019 Gala', note: '2019 Gala for friends and family', fund: 'General 2019 Fund' } ]; + public deleteItem(index: any) { + this.items.splice(index, 1); + } + + public addItem() { + const newItem = { + id: nextItemId++, + title: 'New record ' + nextItemId, + note: 'This is a new record', + fund: 'General 2019 Fund' + }; + this.items.push(newItem); + } + public onCollapse(): void { console.log('Collapsed.'); } From bf31d9d9727c133142a0c7b488320912a0bd1c5f Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Mon, 22 Jul 2019 13:44:41 -0400 Subject: [PATCH 05/14] changed index to id --- .../repeater/repeater-item.component.ts | 20 ++++++------- .../modules/repeater/repeater.component.ts | 15 +++++----- .../modules/repeater/repeater.service.ts | 28 ++++++++++--------- 3 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.ts b/src/app/public/modules/repeater/repeater-item.component.ts index 5a8562c9..3ff8da3d 100644 --- a/src/app/public/modules/repeater/repeater-item.component.ts +++ b/src/app/public/modules/repeater/repeater-item.component.ts @@ -134,16 +134,16 @@ export class SkyRepeaterItemComponent implements OnDestroy { setTimeout(() => { this.repeaterService.addItem(this); // TODO??? - // this.repeaterService.activeItemIndex.subscribe((id: any) => { - // if (id) { - // this.active = this.itemId === id; - // } - // this.changeDetector.markForCheck(); - // }); - - // if (this.active) { - // this.repeaterService.activateItem(this); - // } + this.repeaterService.activeItemId.subscribe((id: string) => { + if (id) { + this.active = this.itemId === id; + } + this.changeDetector.markForCheck(); + }); + + if (this.active) { + this.repeaterService.activateItem(this); + } }); } diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index 4955de36..bf7303e7 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -80,18 +80,19 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest } // Watch for changes to the activeIndex and activate the appropriate item. - this.repeaterService.activeItemIndex + this.repeaterService.activeItemId .distinctUntilChanged() - .subscribe((index) => { + .subscribe((activeItemId) => { // HACK: Not selecting the active item in a timeout causes an error. // https://github.com/angular/angular/issues/6005 setTimeout(() => { - if (index !== this.activeIndex) { - this.activeIndex = index; - } - this.items.forEach((item, i) => { - item.active = i === index ? true : false; + const activeItem = this.items.find(item => { + return item.itemId === activeItemId; }); + const activeIndex = this.items.toArray().indexOf(activeItem); + if (activeIndex !== this.activeIndex) { + this.activeIndex = activeIndex; + } }); }); diff --git a/src/app/public/modules/repeater/repeater.service.ts b/src/app/public/modules/repeater/repeater.service.ts index 0995b5e6..6c49e061 100644 --- a/src/app/public/modules/repeater/repeater.service.ts +++ b/src/app/public/modules/repeater/repeater.service.ts @@ -14,7 +14,7 @@ import { @Injectable() export class SkyRepeaterService { - public activeItemIndex: BehaviorSubject = new BehaviorSubject(undefined); + public activeItemId: BehaviorSubject = new BehaviorSubject(undefined); public itemCollapseStateChange = new EventEmitter(); @@ -24,14 +24,17 @@ export class SkyRepeaterService { this.items.take(1).subscribe(items => { const index = items.indexOf(item); if (index > -1) { - this.activeItemIndex.next(index); + this.activeItemId.next(item.itemId); } }); } public activateItemByIndex(index: number): void { - this.items.take(1).subscribe(() => { - this.activeItemIndex.next(index); + this.items.take(1).subscribe(items => { + const activeItem = items[index]; + if (activeItem) { + this.activeItemId.next(activeItem.itemId); + } }); } @@ -50,19 +53,18 @@ export class SkyRepeaterService { public destroyItem(item: SkyRepeaterItemComponent): void { this.items.take(1).subscribe((items) => { - const index = items.indexOf(item); - if (index > -1) { + const indexOfDestroyedItem = items.indexOf(item); + if (indexOfDestroyedItem > -1) { if (item.active) { - // Try selecting the next tab first. - // If there's no next tab, try selecting the previous one. - let newActiveItem = items[index + 1] || items[index - 1]; + // Try selecting the next item first. + // If there's no next item, try selecting the previous one. + let newActiveItem = items[indexOfDestroyedItem + 1] || items[indexOfDestroyedItem - 1]; /*istanbul ignore else */ if (newActiveItem) { - const newIndex = items.indexOf(newActiveItem); - this.activeItemIndex.next(newIndex); + this.activeItemId.next(newActiveItem.itemId); } } - items.splice(index, 1); + items.splice(indexOfDestroyedItem, 1); } this.items.next(items); }); @@ -70,7 +72,7 @@ export class SkyRepeaterService { public destroy(): void { this.items.complete(); - this.activeItemIndex.complete(); + this.activeItemId.complete(); this.itemCollapseStateChange.complete(); } From 0e086cdaab8b3b83b679353d0e18345eeaa62e34 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Mon, 22 Jul 2019 13:52:37 -0400 Subject: [PATCH 06/14] fixed activeIndex on init --- .../repeater/repeater-item.component.ts | 3 +- .../modules/repeater/repeater.component.ts | 34 ++++++++++--------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.ts b/src/app/public/modules/repeater/repeater-item.component.ts index 3ff8da3d..4fbe0774 100644 --- a/src/app/public/modules/repeater/repeater-item.component.ts +++ b/src/app/public/modules/repeater/repeater-item.component.ts @@ -133,12 +133,11 @@ export class SkyRepeaterItemComponent implements OnDestroy { public initializeItemIndex(): void { setTimeout(() => { this.repeaterService.addItem(this); - // TODO??? this.repeaterService.activeItemId.subscribe((id: string) => { if (id) { this.active = this.itemId === id; + this.changeDetector.markForCheck(); } - this.changeDetector.markForCheck(); }); if (this.active) { diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index bf7303e7..c57bccab 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -74,27 +74,29 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest }); }); - // If activeIndex has been set, call service to activate the appropriate item. + // If activeIndex has been set, call service to activate the appropriate item. + setTimeout(() => { if (this.activeIndex || this.activeIndex === 0) { this.repeaterService.activateItemByIndex(this.activeIndex); } + }); - // Watch for changes to the activeIndex and activate the appropriate item. - this.repeaterService.activeItemId - .distinctUntilChanged() - .subscribe((activeItemId) => { - // HACK: Not selecting the active item in a timeout causes an error. - // https://github.com/angular/angular/issues/6005 - setTimeout(() => { - const activeItem = this.items.find(item => { - return item.itemId === activeItemId; - }); - const activeIndex = this.items.toArray().indexOf(activeItem); - if (activeIndex !== this.activeIndex) { - this.activeIndex = activeIndex; - } + // Watch for changes to the activeIndex and activate the appropriate item. + this.repeaterService.activeItemId + .distinctUntilChanged() + .subscribe((activeItemId) => { + // HACK: Not selecting the active item in a timeout causes an error. + // https://github.com/angular/angular/issues/6005 + setTimeout(() => { + const activeItem = this.items.find(item => { + return item.itemId === activeItemId; }); - }); + const activeIndex = this.items.toArray().indexOf(activeItem); + if (activeIndex !== this.activeIndex) { + this.activeIndex = activeIndex; + } + }); + }); // HACK: Not updating for expand mode in a timeout causes an error. // https://github.com/angular/angular/issues/6005 From a8a975fe0186d4d540ae36e0cf8937b9f67209b5 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Mon, 22 Jul 2019 14:01:11 -0400 Subject: [PATCH 07/14] unsubscribe to observables --- .../repeater/repeater-item.component.ts | 36 ++++++---- .../modules/repeater/repeater.component.ts | 68 ++++++++++++------- 2 files changed, 66 insertions(+), 38 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.ts b/src/app/public/modules/repeater/repeater-item.component.ts index 4fbe0774..a8623ed9 100644 --- a/src/app/public/modules/repeater/repeater-item.component.ts +++ b/src/app/public/modules/repeater/repeater-item.component.ts @@ -25,6 +25,10 @@ import { SkyInlineFormConfig } from '@skyux/inline-form'; +import { + Subject +} from 'rxjs/Subject'; + import { SkyRepeaterService } from './repeater.service'; @@ -103,6 +107,8 @@ export class SkyRepeaterItemComponent implements OnDestroy { public slideDirection: string; + private ngUnsubscribe = new Subject(); + private _isCollapsible = true; private _isExpanded = true; @@ -121,24 +127,30 @@ export class SkyRepeaterItemComponent implements OnDestroy { this.collapse.complete(); this.expand.complete(); this.inlineFormClose.complete(); + + this.ngUnsubscribe.next(); + this.ngUnsubscribe.complete(); + this.repeaterService.destroyItem(this); } - public headerClick() { + public headerClick(): void { if (this.isCollapsible) { this.updateForExpanded(!this.isExpanded, true); } } - public initializeItemIndex(): void { + public initializeItem(): void { setTimeout(() => { this.repeaterService.addItem(this); - this.repeaterService.activeItemId.subscribe((id: string) => { - if (id) { - this.active = this.itemId === id; - this.changeDetector.markForCheck(); - } - }); + this.repeaterService.activeItemId + .takeUntil(this.ngUnsubscribe) + .subscribe((id: string) => { + if (id) { + this.active = this.itemId === id; + this.changeDetector.markForCheck(); + } + }); if (this.active) { this.repeaterService.activateItem(this); @@ -146,11 +158,11 @@ export class SkyRepeaterItemComponent implements OnDestroy { }); } - public chevronDirectionChange(direction: string) { + public chevronDirectionChange(direction: string): void { this.updateForExpanded(direction === 'up', true); } - public updateForExpanded(value: boolean, animate: boolean) { + public updateForExpanded(value: boolean, animate: boolean): void { if (this.isCollapsible === false && value === false) { this.logService.warn( `Setting isExpanded to false when the repeater item is not collapsible @@ -171,7 +183,7 @@ export class SkyRepeaterItemComponent implements OnDestroy { } } - public updateIsSelected(value: SkyCheckboxChange) { + public updateIsSelected(value: SkyCheckboxChange): void { this._isSelected = value.checked; } @@ -179,7 +191,7 @@ export class SkyRepeaterItemComponent implements OnDestroy { this.inlineFormClose.emit(inlineFormCloseArgs); } - private slideForExpanded(animate: boolean) { + private slideForExpanded(animate: boolean): void { this.slideDirection = this.isExpanded ? 'down' : 'up'; } } diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index c57bccab..ccb12e07 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -11,6 +11,10 @@ import { import 'rxjs/add/operator/distinctUntilChanged'; +import { + Subject +} from 'rxjs/Subject'; + import { SkyRepeaterItemComponent } from './repeater-item.component'; @@ -43,46 +47,53 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest @ContentChildren(SkyRepeaterItemComponent) public items: QueryList; + private ngUnsubscribe = new Subject(); + private _expandMode = 'none'; constructor( private repeaterService: SkyRepeaterService ) { - this.repeaterService.itemCollapseStateChange.subscribe((item: SkyRepeaterItemComponent) => { - if (this.expandMode === 'single' && item.isExpanded) { - this.items.forEach((otherItem) => { - if (otherItem !== item && otherItem.isExpanded) { - otherItem.isExpanded = false; - } - }); - } - }); + this.repeaterService.itemCollapseStateChange + .takeUntil(this.ngUnsubscribe) + .subscribe((item: SkyRepeaterItemComponent) => { + if (this.expandMode === 'single' && item.isExpanded) { + this.items.forEach((otherItem) => { + if (otherItem !== item && otherItem.isExpanded) { + otherItem.isExpanded = false; + } + }); + } + }); this.updateForExpandMode(); } public ngAfterContentInit() { - // initialize each item's index (in case items are instantiated out of order). - this.items.forEach(item => item.initializeItemIndex()); - this.items.changes.subscribe((change: QueryList) => { - this.repeaterService.items - .take(1) - .subscribe(currentItems => { - change - .filter(item => currentItems.indexOf(item) < 0) - .forEach(item => item.initializeItemIndex()); + // Initialize each item's index (in case items are instantiated out of order). + this.items.forEach(item => item.initializeItem()); + this.items.changes + .takeUntil(this.ngUnsubscribe) + .subscribe((change: QueryList) => { + this.repeaterService.items + .take(1) + .subscribe(currentItems => { + change + .filter(item => currentItems.indexOf(item) < 0) + .forEach(item => item.initializeItem()); + }); }); - }); - // If activeIndex has been set, call service to activate the appropriate item. + // If activeIndex has been set on init, call service to activate the appropriate item. setTimeout(() => { if (this.activeIndex || this.activeIndex === 0) { this.repeaterService.activateItemByIndex(this.activeIndex); } }); - // Watch for changes to the activeIndex and activate the appropriate item. + // Watch for changes on the service's activeItemId and activate the appropriate item. this.repeaterService.activeItemId + .takeUntil(this.ngUnsubscribe) .distinctUntilChanged() .subscribe((activeItemId) => { // HACK: Not selecting the active item in a timeout causes an error. @@ -100,11 +111,13 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest // HACK: Not updating for expand mode in a timeout causes an error. // https://github.com/angular/angular/issues/6005 - this.items.changes.subscribe(() => { - setTimeout(() => { - this.updateForExpandMode(this.items.last); - }, 0); - }); + this.items.changes + .takeUntil(this.ngUnsubscribe) + .subscribe(() => { + setTimeout(() => { + this.updateForExpandMode(this.items.last); + }, 0); + }); setTimeout(() => { this.updateForExpandMode(); @@ -119,6 +132,9 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest public ngOnDestroy(): void { this.repeaterService.destroy(); + + this.ngUnsubscribe.next(); + this.ngUnsubscribe.complete(); } private updateForExpandMode(itemAdded?: SkyRepeaterItemComponent) { From 4dccb6b2095a3d39170c1fa2bb2ca9f4ad28b9d6 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Mon, 22 Jul 2019 14:54:15 -0400 Subject: [PATCH 08/14] updated tests --- .../modules/paging/paging.component.spec.ts | 14 ---- .../fixtures/repeater.component.fixture.html | 11 +++- .../fixtures/repeater.component.fixture.ts | 4 +- .../repeater/repeater-item.component.ts | 10 +-- .../repeater/repeater.component.spec.ts | 64 +++++++++++++------ .../modules/repeater/repeater.service.ts | 57 +++-------------- .../repeater/repeater-visual.component.html | 8 ++- 7 files changed, 74 insertions(+), 94 deletions(-) diff --git a/src/app/public/modules/paging/paging.component.spec.ts b/src/app/public/modules/paging/paging.component.spec.ts index c19941d2..38f9f476 100644 --- a/src/app/public/modules/paging/paging.component.spec.ts +++ b/src/app/public/modules/paging/paging.component.spec.ts @@ -15,14 +15,6 @@ import { expect } from '@skyux-sdk/testing'; -import { - SkyLibResourcesTestService -} from '@skyux/i18n/testing'; - -import { - SkyLibResourcesService -} from '@skyux/i18n'; - import { PagingTestComponent } from './fixtures/paging.component.fixture'; @@ -43,12 +35,6 @@ describe('Paging component', () => { ], imports: [ SkyPagingModule - ], - providers: [ - { - provide: SkyLibResourcesService, - useClass: SkyLibResourcesTestService - } ] }); diff --git a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html index 816ef9db..df1f1b65 100644 --- a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html +++ b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.html @@ -1,6 +1,8 @@ - + @@ -17,7 +19,10 @@ Title 2 Content 2 - + a diff --git a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts index 8349a83d..dd62a364 100644 --- a/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts +++ b/src/app/public/modules/repeater/fixtures/repeater.component.fixture.ts @@ -13,9 +13,9 @@ import { }) export class RepeaterTestComponent { - public expandMode = 'single'; + public activeIndex: number = undefined; - public isActive = false; + public expandMode = 'single'; public lastItemExpanded: boolean; diff --git a/src/app/public/modules/repeater/repeater-item.component.ts b/src/app/public/modules/repeater/repeater-item.component.ts index a8623ed9..371373de 100644 --- a/src/app/public/modules/repeater/repeater-item.component.ts +++ b/src/app/public/modules/repeater/repeater-item.component.ts @@ -146,15 +146,9 @@ export class SkyRepeaterItemComponent implements OnDestroy { this.repeaterService.activeItemId .takeUntil(this.ngUnsubscribe) .subscribe((id: string) => { - if (id) { - this.active = this.itemId === id; - this.changeDetector.markForCheck(); - } + this.active = this.itemId === id; + this.changeDetector.markForCheck(); }); - - if (this.active) { - this.repeaterService.activateItem(this); - } }); } diff --git a/src/app/public/modules/repeater/repeater.component.spec.ts b/src/app/public/modules/repeater/repeater.component.spec.ts index 3c4ca2c4..0209bea9 100644 --- a/src/app/public/modules/repeater/repeater.component.spec.ts +++ b/src/app/public/modules/repeater/repeater.component.spec.ts @@ -536,30 +536,56 @@ describe('Repeater item component', () => { })); }); - it('should add and remove active css class when isActive value changes', fakeAsync(() => { - let fixture = TestBed.createComponent(RepeaterTestComponent); - let cmp: RepeaterTestComponent = fixture.componentInstance; - let el = fixture.nativeElement; - fixture.detectChanges(); - tick(); + describe('with activeIndex', () => { + let fixture: ComponentFixture; + let cmp: RepeaterTestComponent; + let el: any; - let activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); - expect(activeRepeaterItem.length).toBe(0); + beforeEach(() => { + fixture = TestBed.createComponent(RepeaterTestComponent); + cmp = fixture.componentInstance; + el = fixture.nativeElement; + }); - cmp.isActive = true; - fixture.detectChanges(); - tick(); + function getItems(): HTMLElement[] { + return el.querySelectorAll('.sky-repeater-item'); + } - activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); - expect(activeRepeaterItem.length).toBe(1); + it('should show active item if activeIndex is set on init', fakeAsync(() => { + cmp.activeIndex = 0; + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + tick(); - cmp.isActive = false; - fixture.detectChanges(); - tick(); + let activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(1); + })); - activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); - expect(activeRepeaterItem.length).toBe(0); - })); + it('should add and remove active css class when activeIndex value changes', fakeAsync(() => { + fixture.detectChanges(); + tick(); + + let activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(0); + + cmp.activeIndex = 0; + fixture.detectChanges(); + tick(); + + const items = getItems(); + activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(1); + expect(items[0]).toHaveCssClass('sky-repeater-item-active'); + + cmp.activeIndex = undefined; + fixture.detectChanges(); + tick(); + + activeRepeaterItem = el.querySelectorAll('.sky-repeater-item-active'); + expect(activeRepeaterItem.length).toBe(0); + })); + }); describe('with inline-form', () => { let fixture: ComponentFixture; diff --git a/src/app/public/modules/repeater/repeater.service.ts b/src/app/public/modules/repeater/repeater.service.ts index 6c49e061..3587aef1 100644 --- a/src/app/public/modules/repeater/repeater.service.ts +++ b/src/app/public/modules/repeater/repeater.service.ts @@ -20,34 +20,23 @@ export class SkyRepeaterService { public items: BehaviorSubject> = new BehaviorSubject>([]); - public activateItem(item: SkyRepeaterItemComponent): void { - this.items.take(1).subscribe(items => { - const index = items.indexOf(item); - if (index > -1) { - this.activeItemId.next(item.itemId); - } - }); - } - public activateItemByIndex(index: number): void { - this.items.take(1).subscribe(items => { - const activeItem = items[index]; - if (activeItem) { - this.activeItemId.next(activeItem.itemId); - } - }); + if (index === undefined) { + this.activeItemId.next(undefined); + } else { + this.items.take(1).subscribe(items => { + const activeItem = items[index]; + if (activeItem) { + this.activeItemId.next(activeItem.itemId); + } + }); + } } public addItem(item: SkyRepeaterItemComponent): void { this.items.take(1).subscribe((currentItems) => { - // let lastTabIndex = this.getLastItemIndex(currentItems); - // if (currentItems && (lastTabIndex || lastTabIndex === 0)) { - // item.itemId = lastTabIndex + 1; - // } currentItems.push(item); this.items.next(currentItems); - console.log('add item. new array:'); - console.log(currentItems); }); } @@ -79,30 +68,4 @@ export class SkyRepeaterService { public onItemCollapseStateChange(item: SkyRepeaterItemComponent): void { this.itemCollapseStateChange.emit(item); } - - private getLastItemIndex(tabs: Array): number { - // let result: any = undefined; - // for (let i = 0; i < tabs.length; i++) { - // if (typeof tabs[i].itemId === 'number' && - // (result === undefined || result < tabs[i].itemId)) { - // result = tabs[i].itemId; - // } - // } - return tabs.length; - } - - private getItemByIndex(index: number, items: Array) { - return items[index]; - // for (let i = 0, n = items.length; i < n; i++) { - // let item = items[i]; - // if (item.itemId === index) { - // return item; - // } - // } - // return undefined; - } - - private getIndexByItem(items: Array, item: SkyRepeaterItemComponent): number { - return items.indexOf(item); - } } diff --git a/src/app/visual/repeater/repeater-visual.component.html b/src/app/visual/repeater/repeater-visual.component.html index 954ae0d1..5ef27f13 100644 --- a/src/app/visual/repeater/repeater-visual.component.html +++ b/src/app/visual/repeater/repeater-visual.component.html @@ -24,7 +24,7 @@

Basic repeater

class="app-screenshot sky-margin-stacked-default" id="screenshot-repeater-with-active-item" > -

Repeater with isActive enabled on click

+

Repeater with activeIndex enabled on click

@@ -51,6 +51,12 @@

Repeater with isActive enabled on click

> Add new item + +
-

Repeater with activeIndex enabled on click

+

Repeater with activeIndex set on click

@@ -40,7 +40,7 @@

Repeater with activeIndex enabled on click

type="button" (click)="deleteItem(i)" > - Delete + Delete me @@ -55,11 +55,11 @@

Repeater with activeIndex enabled on click

type="button" (click)="activeIndex = undefined" > - Undefined active + Set activeIndex to undefined
- + diff --git a/src/app/visual/repeater/repeater-visual.component.ts b/src/app/visual/repeater/repeater-visual.component.ts index 5b2c351f..cea6599c 100644 --- a/src/app/visual/repeater/repeater-visual.component.ts +++ b/src/app/visual/repeater/repeater-visual.component.ts @@ -17,7 +17,7 @@ let nextItemId: number = 0; }) export class RepeaterVisualComponent { - public activeIndex: number = 2; + public activeIndex = 0; public activeInlineFormId: number; @@ -39,13 +39,12 @@ export class RepeaterVisualComponent { fund: 'General 2018 Fund' }, { - id: 'abba', + id: 'foobar', title: '2018 Special event', note: 'Special event', fund: '2018 Special Events Fund' }, { - id: 'dabba', title: '2019 Gala', note: '2019 Gala for friends and family', fund: 'General 2019 Fund' @@ -73,7 +72,7 @@ export class RepeaterVisualComponent { id: nextItemId++, title: 'New record ' + nextItemId, note: 'This is a new record', - fund: 'General 2019 Fund' + fund: 'New fund' }; this.items.push(newItem); } From 92f48c62663ac2c80d0672f2dee93727d9f9cd0c Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Tue, 23 Jul 2019 09:17:10 -0400 Subject: [PATCH 10/14] removed behavior subject --- .../modules/repeater/repeater.component.ts | 10 ++--- .../modules/repeater/repeater.service.ts | 37 +++++++------------ 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index ccb12e07..6c48e376 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -75,13 +75,9 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest this.items.changes .takeUntil(this.ngUnsubscribe) .subscribe((change: QueryList) => { - this.repeaterService.items - .take(1) - .subscribe(currentItems => { - change - .filter(item => currentItems.indexOf(item) < 0) - .forEach(item => item.initializeItem()); - }); + change + .filter(item => this.repeaterService.items.indexOf(item) < 0) + .forEach(item => item.initializeItem()); }); // If activeIndex has been set on init, call service to activate the appropriate item. diff --git a/src/app/public/modules/repeater/repeater.service.ts b/src/app/public/modules/repeater/repeater.service.ts index 3587aef1..5dbd13d4 100644 --- a/src/app/public/modules/repeater/repeater.service.ts +++ b/src/app/public/modules/repeater/repeater.service.ts @@ -18,49 +18,40 @@ export class SkyRepeaterService { public itemCollapseStateChange = new EventEmitter(); - public items: BehaviorSubject> = new BehaviorSubject>([]); + public items: Array = new Array(); public activateItemByIndex(index: number): void { if (index === undefined) { this.activeItemId.next(undefined); } else { - this.items.take(1).subscribe(items => { - const activeItem = items[index]; + const activeItem = this.items[index]; if (activeItem) { this.activeItemId.next(activeItem.itemId); } - }); } } public addItem(item: SkyRepeaterItemComponent): void { - this.items.take(1).subscribe((currentItems) => { - currentItems.push(item); - this.items.next(currentItems); - }); + this.items.push(item); } public destroyItem(item: SkyRepeaterItemComponent): void { - this.items.take(1).subscribe((items) => { - const indexOfDestroyedItem = items.indexOf(item); - if (indexOfDestroyedItem > -1) { - if (item.active) { - // Try selecting the next item first. - // If there's no next item, try selecting the previous one. - let newActiveItem = items[indexOfDestroyedItem + 1] || items[indexOfDestroyedItem - 1]; - /*istanbul ignore else */ - if (newActiveItem) { - this.activeItemId.next(newActiveItem.itemId); - } + const indexOfDestroyedItem = this.items.indexOf(item); + if (indexOfDestroyedItem > -1) { + if (item.active) { + // Try selecting the next item first. + // If there's no next item, try selecting the previous one. + let newActiveItem = this.items[indexOfDestroyedItem + 1] || this.items[indexOfDestroyedItem - 1]; + /*istanbul ignore else */ + if (newActiveItem) { + this.activeItemId.next(newActiveItem.itemId); } - items.splice(indexOfDestroyedItem, 1); } - this.items.next(items); - }); + this.items.splice(indexOfDestroyedItem, 1); + } } public destroy(): void { - this.items.complete(); this.activeItemId.complete(); this.itemCollapseStateChange.complete(); } From e715f2e0c6bf96f68c37a33b83621df767b29786 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Thu, 25 Jul 2019 08:59:15 -0400 Subject: [PATCH 11/14] PR feedback --- .../repeater/repeater-item.component.html | 2 +- .../repeater/repeater-item.component.ts | 33 ++++++++++--------- .../modules/repeater/repeater.component.ts | 12 ------- .../modules/repeater/repeater.service.ts | 23 ++++++------- 4 files changed, 30 insertions(+), 40 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.html b/src/app/public/modules/repeater/repeater-item.component.html index b68039fd..3bae3cc3 100644 --- a/src/app/public/modules/repeater/repeater-item.component.html +++ b/src/app/public/modules/repeater/repeater-item.component.html @@ -1,7 +1,7 @@
(); - public active: boolean = false; - public contentId: string = `sky-radio-content-${++nextContentId}`; + public isActive: boolean = false; + public set isCollapsible(value: boolean) { if (this.isCollapsible !== value) { this._isCollapsible = value; @@ -123,6 +124,18 @@ export class SkyRepeaterItemComponent implements OnDestroy { this.slideForExpanded(false); } + public ngOnInit(): void { + setTimeout(() => { + this.repeaterService.registerItem(this); + this.repeaterService.activeItemId + .takeUntil(this.ngUnsubscribe) + .subscribe((id: string) => { + this.isActive = this.itemId === id; + this.changeDetector.markForCheck(); + }); + }); + } + public ngOnDestroy(): void { this.collapse.complete(); this.expand.complete(); @@ -131,7 +144,7 @@ export class SkyRepeaterItemComponent implements OnDestroy { this.ngUnsubscribe.next(); this.ngUnsubscribe.complete(); - this.repeaterService.destroyItem(this); + this.repeaterService.unregisterItem(this); } public headerClick(): void { @@ -140,18 +153,6 @@ export class SkyRepeaterItemComponent implements OnDestroy { } } - public initializeItem(): void { - setTimeout(() => { - this.repeaterService.addItem(this); - this.repeaterService.activeItemId - .takeUntil(this.ngUnsubscribe) - .subscribe((id: string) => { - this.active = this.itemId === id; - this.changeDetector.markForCheck(); - }); - }); - } - public chevronDirectionChange(direction: string): void { this.updateForExpanded(direction === 'up', true); } diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index 6c48e376..d2f91067 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -70,16 +70,6 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest } public ngAfterContentInit() { - // Initialize each item's index (in case items are instantiated out of order). - this.items.forEach(item => item.initializeItem()); - this.items.changes - .takeUntil(this.ngUnsubscribe) - .subscribe((change: QueryList) => { - change - .filter(item => this.repeaterService.items.indexOf(item) < 0) - .forEach(item => item.initializeItem()); - }); - // If activeIndex has been set on init, call service to activate the appropriate item. setTimeout(() => { if (this.activeIndex || this.activeIndex === 0) { @@ -127,8 +117,6 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest } public ngOnDestroy(): void { - this.repeaterService.destroy(); - this.ngUnsubscribe.next(); this.ngUnsubscribe.complete(); } diff --git a/src/app/public/modules/repeater/repeater.service.ts b/src/app/public/modules/repeater/repeater.service.ts index 5dbd13d4..a6878662 100644 --- a/src/app/public/modules/repeater/repeater.service.ts +++ b/src/app/public/modules/repeater/repeater.service.ts @@ -1,6 +1,7 @@ import { EventEmitter, - Injectable + Injectable, + OnDestroy } from '@angular/core'; import { @@ -12,13 +13,18 @@ import { } from './repeater-item.component'; @Injectable() -export class SkyRepeaterService { +export class SkyRepeaterService implements OnDestroy { public activeItemId: BehaviorSubject = new BehaviorSubject(undefined); public itemCollapseStateChange = new EventEmitter(); - public items: Array = new Array(); + public items: SkyRepeaterItemComponent[] = []; + + public ngOnDestroy(): void { + this.activeItemId.complete(); + this.itemCollapseStateChange.complete(); + } public activateItemByIndex(index: number): void { if (index === undefined) { @@ -31,14 +37,14 @@ export class SkyRepeaterService { } } - public addItem(item: SkyRepeaterItemComponent): void { + public registerItem(item: SkyRepeaterItemComponent): void { this.items.push(item); } - public destroyItem(item: SkyRepeaterItemComponent): void { + public unregisterItem(item: SkyRepeaterItemComponent): void { const indexOfDestroyedItem = this.items.indexOf(item); if (indexOfDestroyedItem > -1) { - if (item.active) { + if (item.isActive) { // Try selecting the next item first. // If there's no next item, try selecting the previous one. let newActiveItem = this.items[indexOfDestroyedItem + 1] || this.items[indexOfDestroyedItem - 1]; @@ -51,11 +57,6 @@ export class SkyRepeaterService { } } - public destroy(): void { - this.activeItemId.complete(); - this.itemCollapseStateChange.complete(); - } - public onItemCollapseStateChange(item: SkyRepeaterItemComponent): void { this.itemCollapseStateChange.emit(item); } From 8eb078cddaa6f0f65d34642f16ff2b25a679fe8e Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Thu, 25 Jul 2019 13:26:17 -0400 Subject: [PATCH 12/14] removed repeater-item.itemID and will compare whole object instead --- .../repeater/repeater-item.component.ts | 12 ++++------ .../modules/repeater/repeater.component.ts | 20 ++-------------- .../modules/repeater/repeater.service.ts | 23 ++++++------------- .../repeater/repeater-visual.component.html | 10 ++++---- .../repeater/repeater-visual.component.ts | 22 ++++++++++++++++-- 5 files changed, 38 insertions(+), 49 deletions(-) diff --git a/src/app/public/modules/repeater/repeater-item.component.ts b/src/app/public/modules/repeater/repeater-item.component.ts index c81e563c..673352e7 100644 --- a/src/app/public/modules/repeater/repeater-item.component.ts +++ b/src/app/public/modules/repeater/repeater-item.component.ts @@ -36,8 +36,6 @@ import { let nextContentId: number = 0; -let nextItemId: number = 0; - @Component({ selector: 'sky-repeater-item', styleUrls: ['./repeater-item.component.scss'], @@ -104,8 +102,6 @@ export class SkyRepeaterItemComponent implements OnDestroy, OnInit { return this._isCollapsible; } - public itemId: string = `sky-repeater-item-${++nextItemId}`; - public slideDirection: string; private ngUnsubscribe = new Subject(); @@ -127,12 +123,12 @@ export class SkyRepeaterItemComponent implements OnDestroy, OnInit { public ngOnInit(): void { setTimeout(() => { this.repeaterService.registerItem(this); - this.repeaterService.activeItemId + this.repeaterService.activeItemChange .takeUntil(this.ngUnsubscribe) - .subscribe((id: string) => { - this.isActive = this.itemId === id; + .subscribe((item: SkyRepeaterItemComponent) => { + this.isActive = this === item; this.changeDetector.markForCheck(); - }); + }); }); } diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index d2f91067..ce86b7fd 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -11,6 +11,8 @@ import { import 'rxjs/add/operator/distinctUntilChanged'; +import 'rxjs/add/operator/skip'; + import { Subject } from 'rxjs/Subject'; @@ -77,24 +79,6 @@ export class SkyRepeaterComponent implements AfterContentInit, OnChanges, OnDest } }); - // Watch for changes on the service's activeItemId and activate the appropriate item. - this.repeaterService.activeItemId - .takeUntil(this.ngUnsubscribe) - .distinctUntilChanged() - .subscribe((activeItemId) => { - // HACK: Not selecting the active item in a timeout causes an error. - // https://github.com/angular/angular/issues/6005 - setTimeout(() => { - const activeItem = this.items.find(item => { - return item.itemId === activeItemId; - }); - const activeIndex = this.items.toArray().indexOf(activeItem); - if (activeIndex !== this.activeIndex) { - this.activeIndex = activeIndex; - } - }); - }); - // HACK: Not updating for expand mode in a timeout causes an error. // https://github.com/angular/angular/issues/6005 this.items.changes diff --git a/src/app/public/modules/repeater/repeater.service.ts b/src/app/public/modules/repeater/repeater.service.ts index a6878662..3b3f677c 100644 --- a/src/app/public/modules/repeater/repeater.service.ts +++ b/src/app/public/modules/repeater/repeater.service.ts @@ -15,25 +15,25 @@ import { @Injectable() export class SkyRepeaterService implements OnDestroy { - public activeItemId: BehaviorSubject = new BehaviorSubject(undefined); + public activeItemChange = new BehaviorSubject(undefined); public itemCollapseStateChange = new EventEmitter(); public items: SkyRepeaterItemComponent[] = []; public ngOnDestroy(): void { - this.activeItemId.complete(); + this.activeItemChange.complete(); this.itemCollapseStateChange.complete(); } public activateItemByIndex(index: number): void { if (index === undefined) { - this.activeItemId.next(undefined); + this.activeItemChange.next(undefined); } else { - const activeItem = this.items[index]; - if (activeItem) { - this.activeItemId.next(activeItem.itemId); - } + const activeItem = this.items[index]; + if (activeItem) { + this.activeItemChange.next(activeItem); + } } } @@ -44,15 +44,6 @@ export class SkyRepeaterService implements OnDestroy { public unregisterItem(item: SkyRepeaterItemComponent): void { const indexOfDestroyedItem = this.items.indexOf(item); if (indexOfDestroyedItem > -1) { - if (item.isActive) { - // Try selecting the next item first. - // If there's no next item, try selecting the previous one. - let newActiveItem = this.items[indexOfDestroyedItem + 1] || this.items[indexOfDestroyedItem - 1]; - /*istanbul ignore else */ - if (newActiveItem) { - this.activeItemId.next(newActiveItem.itemId); - } - } this.items.splice(indexOfDestroyedItem, 1); } } diff --git a/src/app/visual/repeater/repeater-visual.component.html b/src/app/visual/repeater/repeater-visual.component.html index 435ecaa4..814df21a 100644 --- a/src/app/visual/repeater/repeater-visual.component.html +++ b/src/app/visual/repeater/repeater-visual.component.html @@ -1,4 +1,4 @@ -
@@ -18,7 +18,7 @@

Basic repeater

Content 2 -
+
-->
Repeater with activeIndex set on click [activeIndex]="activeIndex" > {{ item.title }} @@ -59,7 +59,7 @@

Repeater with activeIndex set on click

-
@@ -279,4 +279,4 @@

Repeater with inline-form

-
+ --> diff --git a/src/app/visual/repeater/repeater-visual.component.ts b/src/app/visual/repeater/repeater-visual.component.ts index cea6599c..db6d6ba9 100644 --- a/src/app/visual/repeater/repeater-visual.component.ts +++ b/src/app/visual/repeater/repeater-visual.component.ts @@ -63,11 +63,25 @@ export class RepeaterVisualComponent { } ]; - public deleteItem(index: any) { + public deleteItem(index: any): void { this.items.splice(index, 1); + + // If active item is removed, try selecting the next item. + // If there's not one, try selecting the previous one. + if (index === this.activeIndex) { + this.activeIndex = undefined; + setTimeout(() => { + if (this.items[index]) { + this.activeIndex = index; + } else if (this.items[index - 1]) { + this.activeIndex = index - 1; + } + }); + } + } - public addItem() { + public addItem(): void { const newItem = { id: nextItemId++, title: 'New record ' + nextItemId, @@ -91,4 +105,8 @@ export class RepeaterVisualComponent { // Form handling would go here } + + public onItemClick(index: number): void { + this.activeIndex = index; + } } From c313db3b6e2725fbdeefdce00865c0a5b473b584 Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Thu, 25 Jul 2019 13:28:23 -0400 Subject: [PATCH 13/14] removed some inadvertent commit lines --- src/app/public/modules/repeater/repeater.component.ts | 2 -- src/app/visual/repeater/repeater-visual.component.html | 8 ++++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index ce86b7fd..a5c77a21 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -11,8 +11,6 @@ import { import 'rxjs/add/operator/distinctUntilChanged'; -import 'rxjs/add/operator/skip'; - import { Subject } from 'rxjs/Subject'; diff --git a/src/app/visual/repeater/repeater-visual.component.html b/src/app/visual/repeater/repeater-visual.component.html index 814df21a..68b43864 100644 --- a/src/app/visual/repeater/repeater-visual.component.html +++ b/src/app/visual/repeater/repeater-visual.component.html @@ -1,4 +1,4 @@ - +
Repeater with activeIndex set on click
- + From c0a408b2a3a4f1febdb7405b5429a2ff978f899d Mon Sep 17 00:00:00 2001 From: Alex Kingman Date: Thu, 25 Jul 2019 13:32:21 -0400 Subject: [PATCH 14/14] removed unnecessary import --- src/app/public/modules/repeater/repeater.component.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/app/public/modules/repeater/repeater.component.ts b/src/app/public/modules/repeater/repeater.component.ts index a5c77a21..65155766 100644 --- a/src/app/public/modules/repeater/repeater.component.ts +++ b/src/app/public/modules/repeater/repeater.component.ts @@ -9,8 +9,6 @@ import { SimpleChanges } from '@angular/core'; -import 'rxjs/add/operator/distinctUntilChanged'; - import { Subject } from 'rxjs/Subject';