From 2a2b7a10dbd8e8ec8431fb77b8b8e2ceb980ce2e Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Fri, 18 Nov 2016 10:38:48 -0800 Subject: [PATCH 1/2] fix(sidenav): resolve promise as false rather than rejecting it when the toggle animation is canceled. also clean up the overly complex promise logic --- src/lib/sidenav/sidenav.spec.ts | 42 +++++++-------- src/lib/sidenav/sidenav.ts | 90 ++++++++++++--------------------- 2 files changed, 48 insertions(+), 84 deletions(-) diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index 7949279ece82..42ce7a879149 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -129,27 +129,23 @@ describe('MdSidenav', () => { let sidenav: MdSidenav = fixture.debugElement .query(By.directive(MdSidenav)).componentInstance; - let openCalled = false; - let openCancelled = false; - let closeCalled = false; + let openFinished: boolean; + let closeFinished: boolean; - sidenav.open().then(() => { - openCalled = true; - }, () => { - openCancelled = true; + sidenav.open().then((finished) => { + openFinished = finished; }); // We do not call transition end, close directly. - sidenav.close().then(() => { - closeCalled = true; + sidenav.close().then((finished) => { + closeFinished = finished; }); endSidenavTransition(fixture); tick(); - expect(openCalled).toBe(false); - expect(openCancelled).toBe(true); - expect(closeCalled).toBe(true); + expect(openFinished).toBe(false); + expect(closeFinished).toBe(true); tick(); })); @@ -158,9 +154,8 @@ describe('MdSidenav', () => { let sidenav: MdSidenav = fixture.debugElement .query(By.directive(MdSidenav)).componentInstance; - let closeCalled = false; - let closeCancelled = false; - let openCalled = false; + let closeFinished: boolean; + let openFinished: boolean; // First, open the sidenav completely. sidenav.open(); @@ -168,22 +163,19 @@ describe('MdSidenav', () => { tick(); // Then close and check behavior. - sidenav.close().then(() => { - closeCalled = true; - }, () => { - closeCancelled = true; + sidenav.close().then((finished) => { + closeFinished = finished; }); // We do not call transition end, open directly. - sidenav.open().then(() => { - openCalled = true; + sidenav.open().then((finished) => { + openFinished = finished; }); endSidenavTransition(fixture); tick(); - expect(closeCalled).toBe(false); - expect(closeCancelled).toBe(true); - expect(openCalled).toBe(true); + expect(closeFinished).toBe(false); + expect(openFinished).toBe(true); tick(); })); @@ -219,7 +211,7 @@ describe('MdSidenav', () => { expect(sidenavEl.classList).not.toContain('md-sidenav-closed'); expect(sidenavEl.classList).toContain('md-sidenav-opened'); - expect((testComponent as any)._openPromise).toBeNull(); + expect((testComponent as any)._toggleAnimationPromise).toBeNull(); }); it('should remove align attr from DOM', () => { diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index e725e01f5ce3..8d6841a4105f 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -106,6 +106,15 @@ export class MdSidenav implements AfterContentInit { /** Event emitted when the sidenav alignment changes. */ @Output('align-changed') onAlignChanged = new EventEmitter(); + /** The current toggle animation promise. `null` if no animation is in progress. */ + private _toggleAnimationPromise: Promise = null; + + /** + * The current toggle animation promise resolution function. + * `null` if no animation is in progress. + */ + private _toggleAnimationPromiseResolve: (value: boolean) => void = null; + /** * @param _elementRef The DOM element reference. Used for transition and width calculation. * If not available we do not hook on transitions. @@ -115,9 +124,9 @@ export class MdSidenav implements AfterContentInit { ngAfterContentInit() { // This can happen when the sidenav is set to opened in the template and the transition // isn't ended. - if (this._openPromise) { - this._openPromiseResolve(); - this._openPromise = null; + if (this._toggleAnimationPromise) { + this._toggleAnimationPromiseResolve(true); + this._toggleAnimationPromise = this._toggleAnimationPromiseResolve = null; } } @@ -134,7 +143,7 @@ export class MdSidenav implements AfterContentInit { /** Open this sidenav, and return a Promise that will resolve when it's fully opened (or get * rejected if it didn't). */ - open(): Promise { + open(): Promise { return this.toggle(true); } @@ -142,7 +151,7 @@ export class MdSidenav implements AfterContentInit { * Close this sidenav, and return a Promise that will resolve when it's fully closed (or get * rejected if it didn't). */ - close(): Promise { + close(): Promise { return this.toggle(false); } @@ -151,20 +160,15 @@ export class MdSidenav implements AfterContentInit { * close() when it's closed. * @param isOpen */ - toggle(isOpen: boolean = !this.opened): Promise { - if (!this.valid) { return Promise.resolve(null); } + toggle(isOpen: boolean = !this.opened): Promise { + if (!this.valid) { return Promise.resolve(true); } // Shortcut it if we're already opened. if (isOpen === this.opened) { - if (!this._transition) { - return Promise.resolve(null); - } else { - return isOpen ? this._openPromise : this._closePromise; - } + return this._toggleAnimationPromise || Promise.resolve(true); } this._opened = isOpen; - this._transition = true; if (isOpen) { this.onOpenStart.emit(); @@ -172,26 +176,15 @@ export class MdSidenav implements AfterContentInit { this.onCloseStart.emit(); } - if (isOpen) { - if (this._openPromise == null) { - this._openPromise = new Promise((resolve, reject) => { - this._openPromiseResolve = resolve; - this._openPromiseReject = reject; - }); - } - return this._openPromise; - } else { - if (this._closePromise == null) { - this._closePromise = new Promise((resolve, reject) => { - this._closePromiseResolve = resolve; - this._closePromiseReject = reject; - }); - } - return this._closePromise; + if (this._toggleAnimationPromise) { + this._toggleAnimationPromiseResolve(false); } + this._toggleAnimationPromise = new Promise(resolve => { + this._toggleAnimationPromiseResolve = resolve; + }); + return this._toggleAnimationPromise; } - /** * When transition has finished, set the internal state for classes and emit the proper event. * The event passed is actually of type TransitionEvent, but that type is not available in @@ -201,43 +194,30 @@ export class MdSidenav implements AfterContentInit { if (transitionEvent.target == this._elementRef.nativeElement // Simpler version to check for prefixes. && transitionEvent.propertyName.endsWith('transform')) { - this._transition = false; if (this._opened) { - if (this._openPromise != null) { - this._openPromiseResolve(); - } - if (this._closePromise != null) { - this._closePromiseReject(); - } - this.onOpen.emit(); } else { - if (this._closePromise != null) { - this._closePromiseResolve(); - } - if (this._openPromise != null) { - this._openPromiseReject(); - } - this.onClose.emit(); } - this._openPromise = null; - this._closePromise = null; + if (this._toggleAnimationPromise) { + this._toggleAnimationPromiseResolve(true); + this._toggleAnimationPromise = this._toggleAnimationPromiseResolve = null; + } } } get _isClosing() { - return !this._opened && this._transition; + return !this._opened && !!this._toggleAnimationPromise; } get _isOpening() { - return this._opened && this._transition; + return this._opened && !!this._toggleAnimationPromise; } get _isClosed() { - return !this._opened && !this._transition; + return !this._opened && !this._toggleAnimationPromise; } get _isOpened() { - return this._opened && !this._transition; + return this._opened && !this._toggleAnimationPromise; } get _isEnd() { return this.align == 'end'; @@ -258,14 +238,6 @@ export class MdSidenav implements AfterContentInit { } return 0; } - - private _transition: boolean = false; - private _openPromise: Promise; - private _openPromiseResolve: () => void; - private _openPromiseReject: () => void; - private _closePromise: Promise; - private _closePromiseResolve: () => void; - private _closePromiseReject: () => void; } /** From dbf585a55f4bad1f833c6195586b40bde8bb4a07 Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Fri, 18 Nov 2016 11:26:18 -0800 Subject: [PATCH 2/2] use a result object instead of boolean as promise result --- src/lib/sidenav/sidenav.spec.ts | 38 ++++++++++++++++++-------------- src/lib/sidenav/sidenav.ts | 39 +++++++++++++++++++++------------ 2 files changed, 46 insertions(+), 31 deletions(-) diff --git a/src/lib/sidenav/sidenav.spec.ts b/src/lib/sidenav/sidenav.spec.ts index 42ce7a879149..bc69e4d9f7fd 100644 --- a/src/lib/sidenav/sidenav.spec.ts +++ b/src/lib/sidenav/sidenav.spec.ts @@ -1,7 +1,7 @@ import {fakeAsync, async, tick, ComponentFixture, TestBed} from '@angular/core/testing'; import {Component} from '@angular/core'; import {By} from '@angular/platform-browser'; -import {MdSidenav, MdSidenavModule} from './sidenav'; +import {MdSidenav, MdSidenavModule, MdSidenavToggleResult} from './sidenav'; function endSidenavTransition(fixture: ComponentFixture) { @@ -129,23 +129,25 @@ describe('MdSidenav', () => { let sidenav: MdSidenav = fixture.debugElement .query(By.directive(MdSidenav)).componentInstance; - let openFinished: boolean; - let closeFinished: boolean; + let openResult: MdSidenavToggleResult; + let closeResult: MdSidenavToggleResult; - sidenav.open().then((finished) => { - openFinished = finished; + sidenav.open().then((result) => { + openResult = result; }); // We do not call transition end, close directly. - sidenav.close().then((finished) => { - closeFinished = finished; + sidenav.close().then((result) => { + closeResult = result; }); endSidenavTransition(fixture); tick(); - expect(openFinished).toBe(false); - expect(closeFinished).toBe(true); + expect(openResult.type).toBe('open'); + expect(openResult.animationFinished).toBe(false); + expect(closeResult.type).toBe('close'); + expect(closeResult.animationFinished).toBe(true); tick(); })); @@ -154,8 +156,8 @@ describe('MdSidenav', () => { let sidenav: MdSidenav = fixture.debugElement .query(By.directive(MdSidenav)).componentInstance; - let closeFinished: boolean; - let openFinished: boolean; + let closeResult: MdSidenavToggleResult; + let openResult: MdSidenavToggleResult; // First, open the sidenav completely. sidenav.open(); @@ -163,19 +165,21 @@ describe('MdSidenav', () => { tick(); // Then close and check behavior. - sidenav.close().then((finished) => { - closeFinished = finished; + sidenav.close().then((result) => { + closeResult = result; }); // We do not call transition end, open directly. - sidenav.open().then((finished) => { - openFinished = finished; + sidenav.open().then((result) => { + openResult = result; }); endSidenavTransition(fixture); tick(); - expect(closeFinished).toBe(false); - expect(openFinished).toBe(true); + expect(closeResult.type).toBe('close'); + expect(closeResult.animationFinished).toBe(false); + expect(openResult.type).toBe('open'); + expect(openResult.animationFinished).toBe(true); tick(); })); diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index 8d6841a4105f..dc13db90b629 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -25,6 +25,13 @@ export class MdDuplicatedSidenavError extends MdError { } } + +/** Sidenav toggle promise result. */ +export class MdSidenavToggleResult { + constructor(public type: 'open' | 'close', public animationFinished: boolean) {} +} + + /** * component. * @@ -107,13 +114,13 @@ export class MdSidenav implements AfterContentInit { @Output('align-changed') onAlignChanged = new EventEmitter(); /** The current toggle animation promise. `null` if no animation is in progress. */ - private _toggleAnimationPromise: Promise = null; + private _toggleAnimationPromise: Promise = null; /** * The current toggle animation promise resolution function. * `null` if no animation is in progress. */ - private _toggleAnimationPromiseResolve: (value: boolean) => void = null; + private _resolveToggleAnimationPromise: (animationFinished: boolean) => void = null; /** * @param _elementRef The DOM element reference. Used for transition and width calculation. @@ -125,8 +132,8 @@ export class MdSidenav implements AfterContentInit { // This can happen when the sidenav is set to opened in the template and the transition // isn't ended. if (this._toggleAnimationPromise) { - this._toggleAnimationPromiseResolve(true); - this._toggleAnimationPromise = this._toggleAnimationPromiseResolve = null; + this._resolveToggleAnimationPromise(true); + this._toggleAnimationPromise = this._resolveToggleAnimationPromise = null; } } @@ -143,7 +150,7 @@ export class MdSidenav implements AfterContentInit { /** Open this sidenav, and return a Promise that will resolve when it's fully opened (or get * rejected if it didn't). */ - open(): Promise { + open(): Promise { return this.toggle(true); } @@ -151,7 +158,7 @@ export class MdSidenav implements AfterContentInit { * Close this sidenav, and return a Promise that will resolve when it's fully closed (or get * rejected if it didn't). */ - close(): Promise { + close(): Promise { return this.toggle(false); } @@ -160,12 +167,15 @@ export class MdSidenav implements AfterContentInit { * close() when it's closed. * @param isOpen */ - toggle(isOpen: boolean = !this.opened): Promise { - if (!this.valid) { return Promise.resolve(true); } + toggle(isOpen: boolean = !this.opened): Promise { + if (!this.valid) { + return Promise.resolve(new MdSidenavToggleResult(isOpen ? 'open' : 'close', true)); + } // Shortcut it if we're already opened. if (isOpen === this.opened) { - return this._toggleAnimationPromise || Promise.resolve(true); + return this._toggleAnimationPromise || + Promise.resolve(new MdSidenavToggleResult(isOpen ? 'open' : 'close', true)); } this._opened = isOpen; @@ -177,10 +187,11 @@ export class MdSidenav implements AfterContentInit { } if (this._toggleAnimationPromise) { - this._toggleAnimationPromiseResolve(false); + this._resolveToggleAnimationPromise(false); } - this._toggleAnimationPromise = new Promise(resolve => { - this._toggleAnimationPromiseResolve = resolve; + this._toggleAnimationPromise = new Promise(resolve => { + this._resolveToggleAnimationPromise = animationFinished => + resolve(new MdSidenavToggleResult(isOpen ? 'open' : 'close', animationFinished)); }); return this._toggleAnimationPromise; } @@ -201,8 +212,8 @@ export class MdSidenav implements AfterContentInit { } if (this._toggleAnimationPromise) { - this._toggleAnimationPromiseResolve(true); - this._toggleAnimationPromise = this._toggleAnimationPromiseResolve = null; + this._resolveToggleAnimationPromise(true); + this._toggleAnimationPromise = this._resolveToggleAnimationPromise = null; } } }