Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sidenav): throw error when sidenav has 2 sidenavs on the same sid… #3369

Merged
merged 1 commit into from
Mar 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/demo-app/sidenav/sidenav-demo.html
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,13 @@ <h2>Sidenav Already Opened</h2>
<h2>Dynamic Alignment Sidenav</h2>

<md-sidenav-container class="demo-sidenav-container">
<md-sidenav #dynamicAlignSidenav mode="push" [align]="side">Drawer</md-sidenav>
<md-sidenav #dynamicAlignSidenav1 mode="side" [align]="invert ? 'end' : 'start'">Start</md-sidenav>
<md-sidenav #dynamicAlignSidenav2 mode="side" [align]="invert ? 'start' : 'end'">End</md-sidenav>

<div class="demo-sidenav-content">
<button (click)="dynamicAlignSidenav.toggle()">Toggle sidenav</button>
<button (click)="side = (side == 'start') ? 'end' : 'start'">Change sides</button>
<button (click)="dynamicAlignSidenav1.toggle(); dynamicAlignSidenav2.toggle()">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this into a method?

Toggle sidenavs
</button>
<button (click)="invert = !invert">Change sides</button>
</div>
</md-sidenav-container>
2 changes: 1 addition & 1 deletion src/demo-app/sidenav/sidenav-demo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ import {Component, ViewEncapsulation} from '@angular/core';
encapsulation: ViewEncapsulation.None,
})
export class SidenavDemo {
side = 'start';
invert = false;
}
4 changes: 0 additions & 4 deletions src/lib/sidenav/sidenav.scss
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,3 @@
}
}
}

.mat-sidenav-invalid {
display: none;
}
18 changes: 10 additions & 8 deletions src/lib/sidenav/sidenav.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,23 +373,25 @@ describe('MdSidenav', () => {
.toBe(false, 'Expected sidenav not to have a native align attribute.');
});

it('should mark sidenavs invalid when multiple have same align', () => {
it('should throw when multiple sidenavs have the same align', () => {
const fixture = TestBed.createComponent(SidenavDynamicAlign);
fixture.detectChanges();

const testComponent: SidenavDynamicAlign = fixture.debugElement.componentInstance;
const sidenavEl = fixture.debugElement.query(By.css('md-sidenav')).nativeElement;
expect(sidenavEl.classList).not.toContain('mat-sidenav-invalid');

testComponent.sidenav1Align = 'end';
fixture.detectChanges();

expect(sidenavEl.classList).toContain('mat-sidenav-invalid');
expect(() => fixture.detectChanges()).toThrow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the specific error?

});

testComponent.sidenav2Align = 'start';
it('should not throw when sidenavs swap sides', () => {
const fixture = TestBed.createComponent(SidenavDynamicAlign);
fixture.detectChanges();

expect(sidenavEl.classList).not.toContain('mat-sidenav-invalid');
const testComponent: SidenavDynamicAlign = fixture.debugElement.componentInstance;
testComponent.sidenav1Align = 'end';
testComponent.sidenav2Align = 'start';

expect(() => fixture.detectChanges()).not.toThrow();
});
});

Expand Down
45 changes: 9 additions & 36 deletions src/lib/sidenav/sidenav.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ export class MdSidenavToggleResult {
'[class.mat-sidenav-over]': '_modeOver',
'[class.mat-sidenav-push]': '_modePush',
'[class.mat-sidenav-side]': '_modeSide',
'[class.mat-sidenav-invalid]': '!valid',
'tabIndex': '-1'
},
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -77,19 +76,6 @@ export class MdSidenav implements AfterContentInit, OnDestroy {
/** Alignment of the sidenav (direction neutral); whether 'start' or 'end'. */
private _align: 'start' | 'end' = 'start';

/** Whether this md-sidenav is part of a valid md-sidenav-container configuration. */
get valid() { return this._valid; }
set valid(value) {
value = coerceBooleanProperty(value);
// When the drawers are not in a valid configuration we close them all until they are in a valid
// configuration again.
if (!value) {
this.close();
}
this._valid = value;
}
private _valid = true;

/** Direction which the sidenav is aligned in. */
@Input()
get align() { return this._align; }
Expand Down Expand Up @@ -221,10 +207,6 @@ export class MdSidenav implements AfterContentInit, OnDestroy {
* @returns Resolves with the result of whether the sidenav was opened or closed.
*/
toggle(isOpen: boolean = !this.opened): Promise<MdSidenavToggleResult> {
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 ||
Expand Down Expand Up @@ -410,25 +392,20 @@ export class MdSidenavContainer implements AfterContentInit {
* changes.
*/
private _watchSidenavAlign(sidenav: MdSidenav): void {
if (!sidenav) { return; }
sidenav.onAlignChanged.subscribe(() => this._validateDrawers());
if (!sidenav) {
return;
}
// NOTE: We need to wait for the microtask queue to be empty before validating,
// since both drawers may be swapping sides at the same time.
sidenav.onAlignChanged.subscribe(() =>
this._ngZone.onMicrotaskEmpty.first().subscribe(() => this._validateDrawers()));
}

/** Toggles the 'mat-sidenav-opened' class on the main 'md-sidenav-container' element. */
private _setContainerClass(sidenav: MdSidenav, bool: boolean): void {
this._renderer.setElementClass(this._element.nativeElement, 'mat-sidenav-opened', bool);
}

/** Sets the valid state of the drawers. */
private _setDrawersValid(valid: boolean) {
this._sidenavs.forEach((sidenav) => {
sidenav.valid = valid;
});
if (!valid) {
this._start = this._end = this._left = this._right = null;
}
}

/** Validate the state of the sidenav children components. */
private _validateDrawers() {
this._start = this._end = null;
Expand All @@ -439,14 +416,12 @@ export class MdSidenavContainer implements AfterContentInit {
for (let sidenav of this._sidenavs.toArray()) {
if (sidenav.align == 'end') {
if (this._end != null) {
this._setDrawersValid(false);
return;
throw new MdDuplicatedSidenavError('end');
}
this._end = sidenav;
} else {
if (this._start != null) {
this._setDrawersValid(false);
return;
throw new MdDuplicatedSidenavError('start');
}
this._start = sidenav;
}
Expand All @@ -462,8 +437,6 @@ export class MdSidenavContainer implements AfterContentInit {
this._left = this._end;
this._right = this._start;
}

this._setDrawersValid(true);
}

_onBackdropClicked() {
Expand Down