From e5258454de0d4d039f93a51830028579af04b4ae Mon Sep 17 00:00:00 2001 From: christophercr Date: Tue, 11 Jun 2019 18:18:48 +0200 Subject: [PATCH] perf(stark-ui): set ChangeDetectionStrategy.OnPush to UI components to prevent Angular from running unnecessary change detection cycles ISSUES CLOSED: #1331 --- .../components/action-bar.component.spec.ts | 93 +++++++++++++------ .../components/action-bar.component.ts | 3 +- .../components/app-footer.component.ts | 5 +- .../app-logo/components/app-logo.component.ts | 3 +- .../components/app-logout.component.ts | 13 ++- .../app-menu-item.component.spec.ts | 87 ++++++++++------- .../components/app-menu-item.component.ts | 3 + .../app-menu/components/app-menu.component.ts | 4 +- .../modules/app-sidebar/app-sidebar.module.ts | 3 +- .../components/breadcrumb.component.ts | 13 ++- .../components/collapsible.component.ts | 14 ++- .../language-selector.component.html | 2 +- .../components/language-selector.component.ts | 33 +++++-- .../minimap/components/minimap.component.ts | 4 +- .../components/pretty-print.component.ts | 14 ++- .../components/route-search.component.ts | 6 +- .../session-card/session-card.component.ts | 4 +- .../slider/components/slider.component.ts | 2 + .../table/components/column.component.ts | 6 ++ .../components/dialogs/multisort.component.ts | 34 +++---- .../table/components/table.component.ts | 50 ++++++++-- 21 files changed, 285 insertions(+), 111 deletions(-) diff --git a/packages/stark-ui/src/modules/action-bar/components/action-bar.component.spec.ts b/packages/stark-ui/src/modules/action-bar/components/action-bar.component.spec.ts index eba74b608a..d64b38298a 100644 --- a/packages/stark-ui/src/modules/action-bar/components/action-bar.component.spec.ts +++ b/packages/stark-ui/src/modules/action-bar/components/action-bar.component.spec.ts @@ -1,4 +1,5 @@ -import { NO_ERRORS_SCHEMA } from "@angular/core"; +/* tslint:disable:completed-docs max-inline-declarations */ +import { Component, NO_ERRORS_SCHEMA, ViewChild } from "@angular/core"; import { async, ComponentFixture, TestBed } from "@angular/core/testing"; import { MatButtonModule } from "@angular/material/button"; import { MatMenuModule } from "@angular/material/menu"; @@ -8,18 +9,43 @@ import { STARK_LOGGING_SERVICE } from "@nationalbankbelgium/stark-core"; import { MockStarkLoggingService } from "@nationalbankbelgium/stark-core/testing"; import { TranslateModule, TranslateService } from "@ngx-translate/core"; import { Subject } from "rxjs"; -import { StarkActionBarComponent } from "./action-bar.component"; +import { StarkActionBarComponent, StarkActionBarComponentMode } from "./action-bar.component"; import { StarkAction } from "./action.intf"; +import { StarkActionBarConfig } from "./action-bar-config.intf"; import createSpy = jasmine.createSpy; describe("ActionBarComponent", () => { - let fixture: ComponentFixture; + @Component({ + selector: "host-component", + template: ` + + ` + }) + class TestHostComponent { + @ViewChild(StarkActionBarComponent) + public starkActionBar!: StarkActionBarComponent; + + public mode?: StarkActionBarComponentMode; + public actionBarId?: string; + public actionBarConfig: StarkActionBarConfig = { actions: [] }; + public alternativeActions?: StarkAction[]; + } + + // IMPORTANT: The official way to test components using ChangeDetectionStrategy.OnPush is to wrap it with a test host component + // see https://github.com/angular/angular/issues/12313#issuecomment-444623173 + let hostFixture: ComponentFixture; + let hostComponent: TestHostComponent; let component: StarkActionBarComponent; const buttonToggleSelector = ".extend-action-bar"; beforeEach(async(() => { return TestBed.configureTestingModule({ - declarations: [StarkActionBarComponent], + declarations: [StarkActionBarComponent, TestHostComponent], imports: [MatButtonModule, MatMenuModule, MatTooltipModule, TranslateModule.forRoot()], providers: [ { provide: STARK_LOGGING_SERVICE, useValue: new MockStarkLoggingService() }, @@ -35,8 +61,8 @@ describe("ActionBarComponent", () => { })); beforeEach(() => { - fixture = TestBed.createComponent(StarkActionBarComponent); - component = fixture.componentInstance; + hostFixture = TestBed.createComponent(TestHostComponent); + hostComponent = hostFixture.componentInstance; const demoActions: StarkAction[] = [ { id: "userDetailValidate", @@ -56,78 +82,87 @@ describe("ActionBarComponent", () => { } ]; - component.actionBarConfig = { + hostComponent.actionBarConfig = { actions: demoActions, isPresent: true }; - fixture.detectChanges(); + hostFixture.detectChanges(); + component = hostComponent.starkActionBar; }); describe("@Input() mode", () => { it("should have the toggle action bar button visible in full mode", () => { - component.mode = "full"; - fixture.detectChanges(); - const buttonToggleExtend: HTMLElement = fixture.nativeElement.querySelector(buttonToggleSelector); + hostComponent.mode = "full"; + hostFixture.detectChanges(); + + const buttonToggleExtend: HTMLElement = hostFixture.nativeElement.querySelector(buttonToggleSelector); expect(buttonToggleExtend).toBeDefined(); }); it("should not have the toggle action bar button visible in compact mode", () => { - component.mode = "compact"; - fixture.detectChanges(); - const buttonToggleExtend: HTMLElement = fixture.nativeElement.querySelector(buttonToggleSelector); + hostComponent.mode = "compact"; + hostFixture.detectChanges(); + + const buttonToggleExtend: HTMLElement = hostFixture.nativeElement.querySelector(buttonToggleSelector); expect(buttonToggleExtend).toBeNull(); }); }); describe("@Input() actionBarId", () => { it("should have set the id of the action bar", () => { - component.actionBarId = "action-bar-id"; - fixture.detectChanges(); - const actionBar: HTMLElement = fixture.nativeElement.querySelector("#" + component.actionBarId); + hostComponent.actionBarId = "action-bar-id"; + hostFixture.detectChanges(); + + const actionBar: HTMLElement = hostFixture.nativeElement.querySelector("#" + hostComponent.actionBarId); expect(actionBar).toBeDefined(); }); }); describe("@Input() actionBarConfig", () => { it("should not call the defined action when disabled", () => { - const menuItem: HTMLElement = fixture.nativeElement.querySelector( - "#" + component.actionBarId + "-" + component.actionBarConfig.actions[0].id + const menuItem: HTMLElement = hostFixture.nativeElement.querySelector( + `#${hostComponent.actionBarId}-${hostComponent.actionBarConfig.actions[0].id}` ); menuItem.click(); - expect(component.actionBarConfig.actions[0].actionCall).not.toHaveBeenCalled(); + expect(hostComponent.actionBarConfig.actions[0].actionCall).not.toHaveBeenCalled(); }); it("should call the defined action when enabled", () => { - const menuItem: HTMLElement = fixture.nativeElement.querySelector( - "#" + component.actionBarId + "-" + component.actionBarConfig.actions[1].id + const menuItem: HTMLElement = hostFixture.nativeElement.querySelector( + `#${hostComponent.actionBarId}-${hostComponent.actionBarConfig.actions[1].id}` ); menuItem.click(); - expect(component.actionBarConfig.actions[1].actionCall).toHaveBeenCalledTimes(1); + expect(hostComponent.actionBarConfig.actions[1].actionCall).toHaveBeenCalledTimes(1); }); }); describe("@Input() alternativeActions", () => { beforeEach(() => { - component.alternativeActions = component.actionBarConfig.actions; - fixture.detectChanges(); + hostComponent.alternativeActions = hostComponent.actionBarConfig.actions; + hostFixture.detectChanges(); }); it("should display", () => { - const actionBar: HTMLElement = fixture.nativeElement.querySelector(".open-alt-actions"); + const actionBar: HTMLElement = hostFixture.nativeElement.querySelector(".open-alt-actions"); expect(actionBar).toBeDefined(); }); }); describe("toggle extended action bar", () => { it("should toggle the action bar extension", () => { - const buttonToggleExtend: HTMLElement = fixture.nativeElement.querySelector(buttonToggleSelector); + hostComponent.mode = "full"; + hostFixture.detectChanges(); + + const buttonToggleExtend: HTMLElement = hostFixture.nativeElement.querySelector(buttonToggleSelector); spyOn(component, "toggleExtendedActionBar").and.callThrough(); buttonToggleExtend.click(); - fixture.detectChanges(); + hostFixture.detectChanges(); + expect(component.toggleExtendedActionBar).toHaveBeenCalledTimes(1); expect(component.isExtended).toBe(true); buttonToggleExtend.click(); - fixture.detectChanges(); + + hostFixture.detectChanges(); expect(component.toggleExtendedActionBar).toHaveBeenCalledTimes(2); expect(component.isExtended).toBe(false); }); diff --git a/packages/stark-ui/src/modules/action-bar/components/action-bar.component.ts b/packages/stark-ui/src/modules/action-bar/components/action-bar.component.ts index 9179530861..b59d37f0de 100644 --- a/packages/stark-ui/src/modules/action-bar/components/action-bar.component.ts +++ b/packages/stark-ui/src/modules/action-bar/components/action-bar.component.ts @@ -1,4 +1,4 @@ -import { Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; +import { ChangeDetectionStrategy, Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; import { StarkActionBarConfig } from "./action-bar-config.intf"; import { StarkAction, StarkActionBarButtonColor } from "./action.intf"; import { STARK_LOGGING_SERVICE, StarkLoggingService } from "@nationalbankbelgium/stark-core"; @@ -18,6 +18,7 @@ const componentName = "stark-action-bar"; selector: "stark-action-bar", templateUrl: "./action-bar.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/app-footer/components/app-footer.component.ts b/packages/stark-ui/src/modules/app-footer/components/app-footer.component.ts index 2b10134531..4722cdafd9 100644 --- a/packages/stark-ui/src/modules/app-footer/components/app-footer.component.ts +++ b/packages/stark-ui/src/modules/app-footer/components/app-footer.component.ts @@ -1,4 +1,4 @@ -import { Component, HostBinding, Inject, Input, OnInit } from "@angular/core"; +import { ChangeDetectionStrategy, Component, HostBinding, Inject, Input, OnInit } from "@angular/core"; import { TranslateService } from "@ngx-translate/core"; import { STARK_LOGGING_SERVICE, StarkLoggingService } from "@nationalbankbelgium/stark-core"; @@ -12,7 +12,8 @@ const componentName = "stark-app-footer"; */ @Component({ selector: "stark-app-footer", - templateUrl: "./app-footer.component.html" + templateUrl: "./app-footer.component.html", + changeDetection: ChangeDetectionStrategy.OnPush }) export class StarkAppFooterComponent implements OnInit { /** diff --git a/packages/stark-ui/src/modules/app-logo/components/app-logo.component.ts b/packages/stark-ui/src/modules/app-logo/components/app-logo.component.ts index 7654e3c2e0..6bdcb17259 100644 --- a/packages/stark-ui/src/modules/app-logo/components/app-logo.component.ts +++ b/packages/stark-ui/src/modules/app-logo/components/app-logo.component.ts @@ -1,4 +1,4 @@ -import { Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; +import { ChangeDetectionStrategy, Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; import { STARK_LOGGING_SERVICE, STARK_ROUTING_SERVICE, StarkLoggingService, StarkRoutingService } from "@nationalbankbelgium/stark-core"; import { AbstractStarkUiComponent } from "../../../common/classes/abstract-component"; @@ -14,6 +14,7 @@ const componentName = "stark-app-logo"; selector: "stark-app-logo", templateUrl: "./app-logo.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/app-logout/components/app-logout.component.ts b/packages/stark-ui/src/modules/app-logout/components/app-logout.component.ts index e3c10a9bda..7af6865e42 100644 --- a/packages/stark-ui/src/modules/app-logout/components/app-logout.component.ts +++ b/packages/stark-ui/src/modules/app-logout/components/app-logout.component.ts @@ -1,4 +1,14 @@ -import { Component, ElementRef, Inject, Input, OnInit, Optional, Renderer2, ViewEncapsulation } from "@angular/core"; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + Inject, + Input, + OnInit, + Optional, + Renderer2, + ViewEncapsulation +} from "@angular/core"; import { STARK_LOGGING_SERVICE, @@ -25,6 +35,7 @@ const componentName = "stark-app-logout"; selector: "stark-app-logout", templateUrl: "./app-logout.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.spec.ts b/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.spec.ts index 152cbd4be7..4853e85094 100644 --- a/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.spec.ts +++ b/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.spec.ts @@ -1,17 +1,41 @@ /*tslint:disable:completed-docs*/ import { NoopAnimationsModule } from "@angular/platform-browser/animations"; import { async, ComponentFixture, TestBed } from "@angular/core/testing"; -import { NO_ERRORS_SCHEMA } from "@angular/compiler/src/core"; +import { Component, NO_ERRORS_SCHEMA, ViewChild } from "@angular/core"; import { UIRouterModule } from "@uirouter/angular"; import { MatExpansionModule } from "@angular/material/expansion"; import { MatListModule } from "@angular/material/list"; import { STARK_LOGGING_SERVICE, STARK_ROUTING_SERVICE } from "@nationalbankbelgium/stark-core"; import { MockStarkLoggingService, MockStarkRoutingService } from "@nationalbankbelgium/stark-core/testing"; import { StarkAppMenuItemComponent } from "./app-menu-item.component"; +import { StarkMenuGroup } from "./app-menu-group.intf"; describe("StarkAppMenuItemComponent", () => { + @Component({ + selector: "host-component", + template: ` + + ` + }) + class TestHostComponent { + @ViewChild(StarkAppMenuItemComponent) + public starkAppMenuItem!: StarkAppMenuItemComponent; + + public level?: number; + public menuGroup: StarkMenuGroup = { + id: "id-item", + label: "Label", + isVisible: true, + isEnabled: true, + targetState: "test" + }; + } + + // IMPORTANT: The official way to test components using ChangeDetectionStrategy.OnPush is to wrap it with a test host component + // see https://github.com/angular/angular/issues/12313#issuecomment-444623173 + let hostComponent: TestHostComponent; let component: StarkAppMenuItemComponent; - let fixture: ComponentFixture; + let hostFixture: ComponentFixture; const mockRoutingService: MockStarkRoutingService = new MockStarkRoutingService(); /** @@ -20,7 +44,7 @@ describe("StarkAppMenuItemComponent", () => { beforeEach(async(() => { return ( TestBed.configureTestingModule({ - declarations: [StarkAppMenuItemComponent], + declarations: [StarkAppMenuItemComponent, TestHostComponent], imports: [MatExpansionModule, MatListModule, NoopAnimationsModule, UIRouterModule], providers: [ { provide: STARK_LOGGING_SERVICE, useValue: new MockStarkLoggingService() }, @@ -39,72 +63,73 @@ describe("StarkAppMenuItemComponent", () => { * Synchronous beforeEach */ beforeEach(() => { - fixture = TestBed.createComponent(StarkAppMenuItemComponent); - component = fixture.componentInstance; - component.menuGroup = { - id: "id-item", - label: "Label", - isVisible: true, - isEnabled: true, - targetState: "test" - }; - component.level = 1; - fixture.detectChanges(); + hostFixture = TestBed.createComponent(TestHostComponent); + hostComponent = hostFixture.componentInstance; + hostComponent.level = 1; + hostFixture.detectChanges(); + component = hostComponent.starkAppMenuItem; mockRoutingService.navigateTo.calls.reset(); }); describe("Simple menu item", () => { it("id should be set", () => { - const item: HTMLElement = fixture.nativeElement.querySelector("#" + component.menuGroup.id); + const item: HTMLElement = hostFixture.nativeElement.querySelector("#" + hostComponent.menuGroup.id); expect(item).toBeDefined(); }); it("should navigate", () => { - const item: HTMLElement = fixture.nativeElement.querySelector("#" + component.menuGroup.id); + const item: HTMLElement = hostFixture.nativeElement.querySelector("#" + hostComponent.menuGroup.id); item.click(); - fixture.detectChanges(); + hostFixture.detectChanges(); + expect(mockRoutingService.navigateTo).toHaveBeenCalledTimes(1); }); it("should be set to disabled", () => { - component.menuGroup.isEnabled = false; - const item: HTMLElement = fixture.nativeElement.querySelector("#" + component.menuGroup.id); - fixture.detectChanges(); + hostComponent.menuGroup = { ...hostComponent.menuGroup, isEnabled: false }; + hostFixture.detectChanges(); + + const item: HTMLElement = hostFixture.nativeElement.querySelector("#" + hostComponent.menuGroup.id); item.click(); - fixture.detectChanges(); + hostFixture.detectChanges(); + expect(item.classList).toContain("stark-disabled"); expect(mockRoutingService.navigateTo).toHaveBeenCalledTimes(0); }); it("should be set to visible", () => { - component.menuGroup.isVisible = false; - fixture.detectChanges(); - const item: HTMLElement = fixture.nativeElement.querySelector("#" + component.menuGroup.id); + hostComponent.menuGroup = { ...hostComponent.menuGroup, isVisible: false }; + hostFixture.detectChanges(); + + const item: HTMLElement = hostFixture.nativeElement.querySelector("#" + hostComponent.menuGroup.id); expect(item).toBeNull(); - component.menuGroup.isVisible = true; - fixture.detectChanges(); + hostComponent.menuGroup = { ...hostComponent.menuGroup, isVisible: true }; + hostFixture.detectChanges(); + expect(item).toBeDefined(); }); it("should dispatch activated event", () => { spyOn(component.activated, "emit"); component.isActive = true; - fixture.detectChanges(); + hostFixture.detectChanges(); + expect(component.activated.emit).toHaveBeenCalledTimes(1); }); it("should dispatch deactivated event", () => { spyOn(component.deactivated, "emit"); component.isActive = false; - fixture.detectChanges(); + hostFixture.detectChanges(); + expect(component.deactivated.emit).toHaveBeenCalledTimes(1); }); }); describe("Menu item with children", () => { beforeEach(() => { - component.menuGroup.entries = [ + hostComponent.menuGroup.entries = [ { id: "id-child-1", label: "Child 1", @@ -120,11 +145,11 @@ describe("StarkAppMenuItemComponent", () => { targetState: "child-2-route" } ]; - fixture.detectChanges(); + hostFixture.detectChanges(); }); it("should have an expansion panel", () => { - const expansionPanel: HTMLElement = fixture.nativeElement.querySelector(".mat-expansion-panel"); + const expansionPanel: HTMLElement = hostFixture.nativeElement.querySelector(".mat-expansion-panel"); expect(expansionPanel).toBeDefined(); }); }); diff --git a/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.ts b/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.ts index db8694ddc0..be717438ef 100644 --- a/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.ts +++ b/packages/stark-ui/src/modules/app-menu/components/app-menu-item.component.ts @@ -1,5 +1,6 @@ import { AfterViewInit, + ChangeDetectionStrategy, Component, ElementRef, EventEmitter, @@ -46,6 +47,8 @@ const DEFAULT_MENU_GROUP: StarkMenuGroup = { selector: "stark-app-menu-item", templateUrl: "./app-menu-item.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, + // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName } diff --git a/packages/stark-ui/src/modules/app-menu/components/app-menu.component.ts b/packages/stark-ui/src/modules/app-menu/components/app-menu.component.ts index 2ac2bced3e..8bbe94af8b 100644 --- a/packages/stark-ui/src/modules/app-menu/components/app-menu.component.ts +++ b/packages/stark-ui/src/modules/app-menu/components/app-menu.component.ts @@ -1,4 +1,4 @@ -import { Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; +import { ChangeDetectionStrategy, Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; import { STARK_LOGGING_SERVICE, STARK_ROUTING_SERVICE, StarkLoggingService, StarkRoutingService } from "@nationalbankbelgium/stark-core"; import { AbstractStarkUiComponent } from "../../../common/classes/abstract-component"; import { StarkMenuSection } from "./app-menu-section.intf"; @@ -17,6 +17,8 @@ const componentName = "stark-app-menu"; selector: "stark-app-menu", templateUrl: "./app-menu.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, + // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName } diff --git a/packages/stark-ui/src/modules/app-sidebar/app-sidebar.module.ts b/packages/stark-ui/src/modules/app-sidebar/app-sidebar.module.ts index 1298c07326..67ac53a84f 100644 --- a/packages/stark-ui/src/modules/app-sidebar/app-sidebar.module.ts +++ b/packages/stark-ui/src/modules/app-sidebar/app-sidebar.module.ts @@ -1,9 +1,8 @@ -import { NgModule, Optional, SkipSelf } from "@angular/core"; +import { ModuleWithProviders, NgModule, Optional, SkipSelf } from "@angular/core"; import { CommonModule } from "@angular/common"; import { MatSidenavModule } from "@angular/material/sidenav"; import { StarkAppSidebarComponent } from "./components"; import { STARK_APP_SIDEBAR_SERVICE, StarkAppSidebarServiceImpl } from "./services"; -import { ModuleWithProviders } from "@angular/compiler/src/core"; @NgModule({ declarations: [StarkAppSidebarComponent], diff --git a/packages/stark-ui/src/modules/breadcrumb/components/breadcrumb.component.ts b/packages/stark-ui/src/modules/breadcrumb/components/breadcrumb.component.ts index c632c13672..7ea3740379 100644 --- a/packages/stark-ui/src/modules/breadcrumb/components/breadcrumb.component.ts +++ b/packages/stark-ui/src/modules/breadcrumb/components/breadcrumb.component.ts @@ -1,7 +1,17 @@ /*tslint:disable:trackBy-function*/ import { StarkBreadcrumbPath } from "./breadcrumb-path.intf"; import { StarkBreadcrumbConfig } from "./breadcrumb-config.intf"; -import { Component, ElementRef, Inject, Input, OnDestroy, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + Inject, + Input, + OnDestroy, + OnInit, + Renderer2, + ViewEncapsulation +} from "@angular/core"; import { STARK_LOGGING_SERVICE, STARK_ROUTING_SERVICE, @@ -23,6 +33,7 @@ const componentName = "stark-breadcrumb"; selector: "stark-breadcrumb", templateUrl: "./breadcrumb.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/collapsible/components/collapsible.component.ts b/packages/stark-ui/src/modules/collapsible/components/collapsible.component.ts index ff74acff96..6d3ea1807c 100644 --- a/packages/stark-ui/src/modules/collapsible/components/collapsible.component.ts +++ b/packages/stark-ui/src/modules/collapsible/components/collapsible.component.ts @@ -1,4 +1,15 @@ -import { Component, ElementRef, EventEmitter, Inject, Input, OnInit, Output, Renderer2, ViewEncapsulation } from "@angular/core"; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + EventEmitter, + Inject, + Input, + OnInit, + Output, + Renderer2, + ViewEncapsulation +} from "@angular/core"; import { STARK_LOGGING_SERVICE, StarkLoggingService } from "@nationalbankbelgium/stark-core"; import { AbstractStarkUiComponent } from "../../../common/classes/abstract-component"; @@ -19,6 +30,7 @@ const DEFAULT_COLLAPSIBLE_ICON = "chevron-right"; selector: "stark-collapsible", templateUrl: "./collapsible.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/language-selector/components/language-selector.component.html b/packages/stark-ui/src/modules/language-selector/components/language-selector.component.html index 9efbc189b7..e1d65c76ad 100644 --- a/packages/stark-ui/src/modules/language-selector/components/language-selector.component.html +++ b/packages/stark-ui/src/modules/language-selector/components/language-selector.component.html @@ -1,7 +1,7 @@
, protected renderer: Renderer2, - protected elementRef: ElementRef + protected elementRef: ElementRef, + protected cdRef: ChangeDetectorRef ) { super(renderer, elementRef); } @@ -82,12 +96,13 @@ export class StarkLanguageSelectorComponent extends AbstractStarkUiComponent imp public ngOnInit(): void { this.logger.debug(componentName + ": controller initialized"); - this.sessionServiceSubscription = this.sessionService - .getCurrentLanguage() - .subscribe( - (language: string) => (this.selectedLanguage = language), - () => this.logger.error(componentName + ": an error occurred getting the current language.") - ); + this.sessionServiceSubscription = this.sessionService.getCurrentLanguage().subscribe( + (language: string) => { + this.selectedLanguage = language; + this.cdRef.detectChanges(); // necessary because of the ChangeDetectionStrategy.OnPush + }, + () => this.logger.error(componentName + ": an error occurred getting the current language.") + ); } /** diff --git a/packages/stark-ui/src/modules/minimap/components/minimap.component.ts b/packages/stark-ui/src/modules/minimap/components/minimap.component.ts index 5d6c9e0ab6..ad4a743bf6 100644 --- a/packages/stark-ui/src/modules/minimap/components/minimap.component.ts +++ b/packages/stark-ui/src/modules/minimap/components/minimap.component.ts @@ -1,4 +1,4 @@ -import { Component, ElementRef, EventEmitter, Input, Output, Renderer2, ViewEncapsulation } from "@angular/core"; +import { ChangeDetectionStrategy, Component, ElementRef, EventEmitter, Input, Output, Renderer2, ViewEncapsulation } from "@angular/core"; import { StarkMinimapItemProperties } from "./item-properties.intf"; import { AbstractStarkUiComponent } from "../../../common/classes/abstract-component"; @@ -17,6 +17,8 @@ const componentName = "stark-minimap"; selector: "stark-minimap", templateUrl: "./minimap.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, + // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName } diff --git a/packages/stark-ui/src/modules/pretty-print/components/pretty-print.component.ts b/packages/stark-ui/src/modules/pretty-print/components/pretty-print.component.ts index 5555ec3f61..f35f6fb314 100644 --- a/packages/stark-ui/src/modules/pretty-print/components/pretty-print.component.ts +++ b/packages/stark-ui/src/modules/pretty-print/components/pretty-print.component.ts @@ -1,4 +1,15 @@ -import { Component, ElementRef, Inject, Input, OnChanges, OnInit, Renderer2, SimpleChanges, ViewEncapsulation } from "@angular/core"; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + Inject, + Input, + OnChanges, + OnInit, + Renderer2, + SimpleChanges, + ViewEncapsulation +} from "@angular/core"; /* tslint:disable:no-duplicate-imports no-import-side-effect */ import * as Prism from "prismjs"; import { Grammar } from "prismjs"; @@ -65,6 +76,7 @@ export type StarkPrettyPrintFormat = "css" | "scss" | "html" | "xml" | "json" | selector: "stark-pretty-print", templateUrl: "./pretty-print.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/route-search/components/route-search.component.ts b/packages/stark-ui/src/modules/route-search/components/route-search.component.ts index 09a62a30c3..fb3be2341e 100644 --- a/packages/stark-ui/src/modules/route-search/components/route-search.component.ts +++ b/packages/stark-ui/src/modules/route-search/components/route-search.component.ts @@ -1,4 +1,4 @@ -import { Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; +import { ChangeDetectionStrategy, Component, ElementRef, Inject, Input, OnInit, Renderer2, ViewEncapsulation } from "@angular/core"; import { FormControl } from "@angular/forms"; import { LangChangeEvent, TranslateService } from "@ngx-translate/core"; import { Ng2StateDeclaration } from "@uirouter/angular"; @@ -15,7 +15,7 @@ import { import { AbstractStarkUiComponent } from "../../../common/classes/abstract-component"; import { StarkRouteSearchEntry } from "./route-search-entry.intf"; import { StarkMenuConfig, StarkMenuGroup } from "../../app-menu/components"; -import sortBy from "lodash-es/sortBy"; +import sortBy from "lodash-es/sortBy"; /** * Name of the component @@ -47,6 +47,8 @@ export type StarkRouteSearchDirection = "left" | "right"; selector: "stark-route-search", templateUrl: "./route-search.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, + // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName } diff --git a/packages/stark-ui/src/modules/session-ui/components/session-card/session-card.component.ts b/packages/stark-ui/src/modules/session-ui/components/session-card/session-card.component.ts index 3bdbc39c24..6fa1a88fa8 100644 --- a/packages/stark-ui/src/modules/session-ui/components/session-card/session-card.component.ts +++ b/packages/stark-ui/src/modules/session-ui/components/session-card/session-card.component.ts @@ -1,4 +1,4 @@ -import { Component, Input, ViewEncapsulation } from "@angular/core"; +import { ChangeDetectionStrategy, Component, Input, ViewEncapsulation } from "@angular/core"; /** * The name of the component @@ -12,6 +12,8 @@ const componentName = "stark-session-card"; selector: "stark-session-card", templateUrl: "./session-card.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, + // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName } diff --git a/packages/stark-ui/src/modules/slider/components/slider.component.ts b/packages/stark-ui/src/modules/slider/components/slider.component.ts index dc92aedf70..7fb18b41cc 100644 --- a/packages/stark-ui/src/modules/slider/components/slider.component.ts +++ b/packages/stark-ui/src/modules/slider/components/slider.component.ts @@ -1,6 +1,7 @@ import isEqual from "lodash-es/isEqual"; import { AfterViewInit, + ChangeDetectionStrategy, Component, ElementRef, EventEmitter, @@ -31,6 +32,7 @@ const componentName = "stark-slider"; selector: "stark-slider", templateUrl: "./slider.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName diff --git a/packages/stark-ui/src/modules/table/components/column.component.ts b/packages/stark-ui/src/modules/table/components/column.component.ts index 297e6214fb..18f43a0d0d 100644 --- a/packages/stark-ui/src/modules/table/components/column.component.ts +++ b/packages/stark-ui/src/modules/table/components/column.component.ts @@ -1,4 +1,5 @@ import { + ChangeDetectionStrategy, Component, ContentChild, ElementRef, @@ -28,6 +29,7 @@ import { StarkColumnFilterChangedOutput, StarkColumnSortChangedOutput, StarkTabl selector: "stark-table-column", templateUrl: "./column.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: "stark-table-column" @@ -116,6 +118,10 @@ export class StarkTableColumnComponent extends AbstractStarkUiComponent implemen return this._headerLabel || this.name; } + /** + * @ignore + * @internal + */ private _headerLabel?: string; /** diff --git a/packages/stark-ui/src/modules/table/components/dialogs/multisort.component.ts b/packages/stark-ui/src/modules/table/components/dialogs/multisort.component.ts index 666c4632dc..cef620ca71 100644 --- a/packages/stark-ui/src/modules/table/components/dialogs/multisort.component.ts +++ b/packages/stark-ui/src/modules/table/components/dialogs/multisort.component.ts @@ -114,12 +114,12 @@ export class StarkTableMultisortDialogComponent extends AbstractStarkUiComponent /** * Called when Delete button is clicked. - * Deletes the sorting rule corresponding to the given column. - * @param column - The column to be deleted + * Deletes the given column sorting rule. + * @param rule - The column sorting rule to be deleted */ - public onDelete(column: StarkSortingRule): void { - column.sortDirection = ""; - column.sortPriority = 100; + public onDelete(rule: StarkSortingRule): void { + rule.sortDirection = ""; + rule.sortPriority = 100; this.sortRules(); } @@ -133,28 +133,22 @@ export class StarkTableMultisortDialogComponent extends AbstractStarkUiComponent /** * Called when the sorting rule changes. - * @param oldColumn - New state of the sorting rule - * @param newColumn - Previous state of the sorting rule - */ - public onColumnChange(oldColumn: StarkSortingRule, newColumn: StarkSortingRule): void { - newColumn.sortDirection = oldColumn.sortDirection; - newColumn.sortPriority = oldColumn.sortPriority; - oldColumn.sortDirection = ""; - oldColumn.sortPriority = 100; + * @param oldRule - New state of the sorting rule + * @param newRule - Previous state of the sorting rule + */ + public onColumnChange(oldRule: StarkSortingRule, newRule: StarkSortingRule): void { + newRule.sortDirection = oldRule.sortDirection; + newRule.sortPriority = oldRule.sortPriority; + oldRule.sortDirection = ""; + oldRule.sortPriority = 100; this.sortRules(); } /** * Called when Save button is clicked. - * Updates all the original sorting rules with the config defined in this dialog and closes the dialog passing the updated rules + * Close the dialog returning the updated rules defined back to the dialog opener. */ public onSave(): void { - this.currentPriority = 1; - for (const rule of this.rules) { - rule.column.sortDirection = rule.sortDirection; - rule.column.sortPriority = this.currentPriority; - this.currentPriority++; - } this.dialogRef.close(this.rules); } diff --git a/packages/stark-ui/src/modules/table/components/table.component.ts b/packages/stark-ui/src/modules/table/components/table.component.ts index 04ef00c340..b0bf8ab5af 100644 --- a/packages/stark-ui/src/modules/table/components/table.component.ts +++ b/packages/stark-ui/src/modules/table/components/table.component.ts @@ -1,6 +1,7 @@ import { AfterContentInit, AfterViewInit, + ChangeDetectionStrategy, ChangeDetectorRef, Component, ContentChildren, @@ -72,6 +73,7 @@ const DEFAULT_COLUMN_PROPERTIES: Partial = { selector: "stark-table", templateUrl: "./table.component.html", encapsulation: ViewEncapsulation.None, + changeDetection: ChangeDetectionStrategy.OnPush, // We need to use host instead of @HostBinding: https://github.com/NationalBankBelgium/stark/issues/664 host: { class: componentName @@ -104,19 +106,29 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI return this._columnProperties; } + /** + * Return the array of {@link StarkMinimapItemProperties} to be displayed display in the table minimap component + */ public get _minimapItemProperties(): StarkMinimapItemProperties[] { - return this._columnProperties.map(({ name, label }: StarkTableColumnProperties) => ({ + return this.columnProperties.map(({ name, label }: StarkTableColumnProperties) => ({ name, label: label || name })); } + /** + * Return the items to be shown as "visible" in the table minimap component + */ public get _visibleMinimapItems(): string[] { - return this._columnProperties + return this.columnProperties .filter(({ isVisible }: StarkTableColumnProperties) => isVisible) .map(({ name }: StarkTableColumnProperties) => name); } + /** + * @ignore + * @internal + */ private _columnProperties: StarkTableColumnProperties[] = []; /** @@ -354,6 +366,10 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI */ public displayedColumns: string[] = []; + /** + * @ignore + * @internal + */ public _globalFilterFormCtrl = new FormControl(); /** @@ -766,7 +782,30 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI }); dialogRef.afterClosed().subscribe((savedRules: StarkSortingRule[] | undefined) => { + // re-calculate the orderProperties with the sorting defined in the dialog if (savedRules) { + const newOrderProperties: string[] = []; + + // IMPORTANT: the rules should be ordered by priority because the priority passed to the columns by getColumnSortingPriority() + // is calculated based on the order in which the columns appear in the "orderProperties" + const orderedRulesByPriority = savedRules + // we only care about the columns that are really sorted, the rest should be discarded + .filter((rule: StarkSortingRule) => rule.sortDirection !== "") + .sort((rule1: StarkSortingRule, rule2: StarkSortingRule) => { + return rule1.sortPriority < rule2.sortPriority ? -1 : 1; + }); + + for (const rule of orderedRulesByPriority) { + let columnWithSortDirection: string = rule.column.name; // asc + if (rule.sortDirection === "desc") { + columnWithSortDirection = "-" + rule.column.name; // desc + } + newOrderProperties.push(columnWithSortDirection); + } + + this.orderProperties = newOrderProperties; // enforcing immutability :) + this.cdRef.detectChanges(); // needed due to ChangeDetectionStrategy.OnPush in order to refresh the columns + this.sortData(); } }); @@ -806,7 +845,6 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI * In case there is a compareFn defined for any of the columns then such method is called to perform the custom sorting. * FIXME: refactor this method to reduce its cognitive complexity */ - /* tslint:disable-next-line:cognitive-complexity */ public sortData(): void { if (!this.columns) { @@ -900,16 +938,16 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI */ public getColumnSortingPriority(columnName: string): number | undefined { let columnSortingPriority: number | undefined; - let index = 1; + let priority = 1; if (this.isArrayFilled(this.orderProperties)) { for (const orderProperty of this.orderProperties) { if (orderProperty === columnName || (orderProperty.startsWith("-") && orderProperty === "-" + columnName)) { - columnSortingPriority = index; + columnSortingPriority = priority; break; } - index++; + priority++; } }