From 5d0e3d772bbaeb1349faba2b786b1787442652b1 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 21 Feb 2017 20:21:30 +0100 Subject: [PATCH 1/4] refactor(connected-overlay-directive): avoid issues with property initialization * Refactors the `ConnectedOverlayDirective` not to depend on the order in which the `open` and `origin` properties are set. * Moves the `open`, `offsetX` and `offsetY` properties from using setters to `ngOnChanges` since the latter tends to output less JS. Fixes #3225. --- .../core/overlay/overlay-directives.spec.ts | 4 ++++ src/lib/core/overlay/overlay-directives.ts | 24 +++++++++---------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/lib/core/overlay/overlay-directives.spec.ts b/src/lib/core/overlay/overlay-directives.spec.ts index 7cb58b1ff26d..97be07ab4514 100644 --- a/src/lib/core/overlay/overlay-directives.spec.ts +++ b/src/lib/core/overlay/overlay-directives.spec.ts @@ -194,6 +194,8 @@ describe('Overlay directives', () => { fixture.detectChanges(); fixture.componentInstance.offsetX = 15; + fixture.detectChanges(); + fixture.componentInstance.isOpen = true; fixture.detectChanges(); @@ -223,6 +225,8 @@ describe('Overlay directives', () => { fixture.detectChanges(); fixture.componentInstance.offsetY = 55; + fixture.detectChanges(); + fixture.componentInstance.isOpen = true; fixture.detectChanges(); expect(pane.style.top) diff --git a/src/lib/core/overlay/overlay-directives.ts b/src/lib/core/overlay/overlay-directives.ts index b3ab073e02e6..dbc2c0e1a851 100644 --- a/src/lib/core/overlay/overlay-directives.ts +++ b/src/lib/core/overlay/overlay-directives.ts @@ -10,6 +10,8 @@ import { Output, ElementRef, Renderer2, + OnChanges, + SimpleChanges, } from '@angular/core'; import {Overlay, OVERLAY_PROVIDERS} from './overlay'; import {OverlayRef} from './overlay-ref'; @@ -63,10 +65,9 @@ export class OverlayOrigin { selector: '[cdk-connected-overlay], [connected-overlay], [cdkConnectedOverlay]', exportAs: 'cdkConnectedOverlay' }) -export class ConnectedOverlayDirective implements OnDestroy { +export class ConnectedOverlayDirective implements OnDestroy, OnChanges { private _overlayRef: OverlayRef; private _templatePortal: TemplatePortal; - private _open = false; private _hasBackdrop = false; private _backdropSubscription: Subscription; private _positionSubscription: Subscription; @@ -125,6 +126,9 @@ export class ConnectedOverlayDirective implements OnDestroy { /** Strategy to be used when handling scroll events while the overlay is open. */ @Input() scrollStrategy: ScrollStrategy = new RepositionScrollStrategy(this._scrollDispatcher); + /** Whether the overlay is open. */ + @Input() open: boolean = false; + /** Whether or not the overlay should attach a backdrop. */ @Input() get hasBackdrop() { @@ -135,16 +139,6 @@ export class ConnectedOverlayDirective implements OnDestroy { this._hasBackdrop = coerceBooleanProperty(value); } - @Input() - get open() { - return this._open; - } - - set open(value: boolean) { - value ? this._attachOverlay() : this._detachOverlay(); - this._open = value; - } - /** Event emitted when the backdrop is clicked. */ @Output() backdropClick = new EventEmitter(); @@ -183,6 +177,12 @@ export class ConnectedOverlayDirective implements OnDestroy { this._destroyOverlay(); } + ngOnChanges(changes: SimpleChanges) { + if (changes['open']) { + changes['open'].currentValue ? this._attachOverlay() : this._detachOverlay(); + } + } + /** Creates an overlay */ private _createOverlay() { if (!this.positions || !this.positions.length) { From b2225e25dbf668fc524e9a16f2c492d45d60f18f Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 4 Mar 2017 09:07:09 +0100 Subject: [PATCH 2/4] fix: offset changes not being deteched in md-select --- src/lib/core/overlay/overlay-directives.ts | 2 +- src/lib/select/select.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lib/core/overlay/overlay-directives.ts b/src/lib/core/overlay/overlay-directives.ts index dbc2c0e1a851..aefd5c200795 100644 --- a/src/lib/core/overlay/overlay-directives.ts +++ b/src/lib/core/overlay/overlay-directives.ts @@ -179,7 +179,7 @@ export class ConnectedOverlayDirective implements OnDestroy, OnChanges { ngOnChanges(changes: SimpleChanges) { if (changes['open']) { - changes['open'].currentValue ? this._attachOverlay() : this._detachOverlay(); + this.open ? this._attachOverlay() : this._detachOverlay(); } } diff --git a/src/lib/select/select.ts b/src/lib/select/select.ts index 737ba26901d8..ad22805415b2 100644 --- a/src/lib/select/select.ts +++ b/src/lib/select/select.ts @@ -748,6 +748,10 @@ export class MdSelect implements AfterContentInit, OnDestroy, OnInit, ControlVal } this._checkOverlayWithinViewport(maxScroll); + + // Change detection needs to be triggered in order for + // the overlay's position to get updated in time. + this._changeDetectorRef.detectChanges(); } /** From 6a1327c20b7657efcccb80f5a217fe1c0c4731bb Mon Sep 17 00:00:00 2001 From: crisbeto Date: Tue, 9 May 2017 21:20:43 +0200 Subject: [PATCH 3/4] chore: revert redundant calls --- src/lib/core/overlay/overlay-directives.spec.ts | 4 ---- src/lib/select/select.ts | 4 ---- 2 files changed, 8 deletions(-) diff --git a/src/lib/core/overlay/overlay-directives.spec.ts b/src/lib/core/overlay/overlay-directives.spec.ts index 97be07ab4514..7cb58b1ff26d 100644 --- a/src/lib/core/overlay/overlay-directives.spec.ts +++ b/src/lib/core/overlay/overlay-directives.spec.ts @@ -194,8 +194,6 @@ describe('Overlay directives', () => { fixture.detectChanges(); fixture.componentInstance.offsetX = 15; - fixture.detectChanges(); - fixture.componentInstance.isOpen = true; fixture.detectChanges(); @@ -225,8 +223,6 @@ describe('Overlay directives', () => { fixture.detectChanges(); fixture.componentInstance.offsetY = 55; - fixture.detectChanges(); - fixture.componentInstance.isOpen = true; fixture.detectChanges(); expect(pane.style.top) diff --git a/src/lib/select/select.ts b/src/lib/select/select.ts index ad22805415b2..737ba26901d8 100644 --- a/src/lib/select/select.ts +++ b/src/lib/select/select.ts @@ -748,10 +748,6 @@ export class MdSelect implements AfterContentInit, OnDestroy, OnInit, ControlVal } this._checkOverlayWithinViewport(maxScroll); - - // Change detection needs to be triggered in order for - // the overlay's position to get updated in time. - this._changeDetectorRef.detectChanges(); } /** From 4b4b7346e21cc6a82eab6a66d2f0ab60e25a3edb Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 11 May 2017 20:06:08 +0200 Subject: [PATCH 4/4] fix: add test case --- .../core/overlay/overlay-directives.spec.ts | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/lib/core/overlay/overlay-directives.spec.ts b/src/lib/core/overlay/overlay-directives.spec.ts index 7cb58b1ff26d..2daece1e33d4 100644 --- a/src/lib/core/overlay/overlay-directives.spec.ts +++ b/src/lib/core/overlay/overlay-directives.spec.ts @@ -1,7 +1,7 @@ -import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {ComponentFixture, TestBed, async} from '@angular/core/testing'; import {Component, ViewChild} from '@angular/core'; import {By} from '@angular/platform-browser'; -import {ConnectedOverlayDirective, OverlayModule} from './overlay-directives'; +import {ConnectedOverlayDirective, OverlayModule, OverlayOrigin} from './overlay-directives'; import {OverlayContainer} from './overlay-container'; import {ConnectedPositionStrategy} from './position/connected-position-strategy'; import {ConnectedOverlayPositionChange} from './position/connected-position'; @@ -18,7 +18,7 @@ describe('Overlay directives', () => { beforeEach(() => { TestBed.configureTestingModule({ imports: [OverlayModule], - declarations: [ConnectedOverlayDirectiveTest], + declarations: [ConnectedOverlayDirectiveTest, ConnectedOverlayPropertyInitOrder], providers: [ {provide: OverlayContainer, useFactory: () => { overlayContainerElement = document.createElement('div'); @@ -111,6 +111,21 @@ describe('Overlay directives', () => { 'Expected overlay to have been detached.'); }); + it('should not depend on the order in which the `origin` and `open` are set', async(() => { + fixture.destroy(); + + const propOrderFixture = TestBed.createComponent(ConnectedOverlayPropertyInitOrder); + propOrderFixture.detectChanges(); + + const overlayDirective = propOrderFixture.componentInstance.connectedOverlayDirective; + + expect(() => { + overlayDirective.open = true; + overlayDirective.origin = propOrderFixture.componentInstance.trigger; + propOrderFixture.detectChanges(); + }).not.toThrow(); + })); + describe('inputs', () => { it('should set the width', () => { @@ -310,3 +325,14 @@ class ConnectedOverlayDirectiveTest { @ViewChild(ConnectedOverlayDirective) connectedOverlayDirective: ConnectedOverlayDirective; } + +@Component({ + template: ` + + Menu content`, +}) +class ConnectedOverlayPropertyInitOrder { + @ViewChild(ConnectedOverlayDirective) connectedOverlayDirective: ConnectedOverlayDirective; + @ViewChild('trigger') trigger: OverlayOrigin; +} +