Skip to content

Commit

Permalink
fix(menu): unable to bind to xPosition and yPosition (#4213)
Browse files Browse the repository at this point in the history 
Fixes not being able to use data bindings on the `xPosition` and `yPosition` properties.

**Note:** These changes could be considered breaking, if somebody was implementing the `MdMenuPanel` interface.

Fixes #4169.
  • Loading branch information
@crisbeto @andrewseguin
crisbeto authored and andrewseguin committed May 2, 2017
1 parent b4e8c7d commit 1fd50aa
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 59 deletions.
71 changes: 31 additions & 40 deletions src/lib/menu/menu-directive.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import {
AfterContentInit,
Attribute,
Component,
ContentChildren,
EventEmitter,
Expand Down Expand Up @@ -37,6 +36,8 @@ import {transformMenu, fadeInItems} from './menu-animations';
})
export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
private _keyManager: FocusKeyManager;
private _xPosition: MenuPositionX = 'after';
private _yPosition: MenuPositionY = 'below';

/** Subscription to tab events on the menu panel */
private _tabSubscription: Subscription;
Expand All @@ -45,34 +46,38 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
_classList: any = {};

/** Position of the menu in the X axis. */
positionX: MenuPositionX = 'after';
@Input()
get xPosition() { return this._xPosition; }
set xPosition(value: MenuPositionX) {
if (value !== 'before' && value !== 'after') {
throw new MdMenuInvalidPositionX();
}
this._xPosition = value;
this.setPositionClasses();
}

/** Position of the menu in the Y axis. */
positionY: MenuPositionY = 'below';
@Input()
get yPosition() { return this._yPosition; }
set yPosition(value: MenuPositionY) {
if (value !== 'above' && value !== 'below') {
throw new MdMenuInvalidPositionY();
}
this._yPosition = value;
this.setPositionClasses();
}

@ViewChild(TemplateRef) templateRef: TemplateRef<any>;
@ContentChildren(MdMenuItem) items: QueryList<MdMenuItem>;
@Input() overlapTrigger = true;

constructor(@Attribute('xPosition') posX: MenuPositionX,
@Attribute('yPosition') posY: MenuPositionY,
@Attribute('x-position') deprecatedPosX: MenuPositionX,
@Attribute('y-position') deprecatedPosY: MenuPositionY) {

// TODO(kara): Remove kebab-case attributes after next release
if (deprecatedPosX) { this._setPositionX(deprecatedPosX); }
if (deprecatedPosY) { this._setPositionY(deprecatedPosY); }
/** List of the items inside of a menu. */
@ContentChildren(MdMenuItem) items: QueryList<MdMenuItem>;

if (posX) { this._setPositionX(posX); }
if (posY) { this._setPositionY(posY); }
this.setPositionClasses(this.positionX, this.positionY);
}
/** Whether the menu should overlap its trigger. */
@Input() overlapTrigger = true;

ngAfterContentInit() {
this._keyManager = new FocusKeyManager(this.items).withWrap();
this._tabSubscription = this._keyManager.tabOut.subscribe(() => {
this._emitCloseEvent();
});
this._tabSubscription = this._keyManager.tabOut.subscribe(() => this._emitCloseEvent());
}

ngOnDestroy() {
Expand All @@ -93,7 +98,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
obj[className] = true;
return obj;
}, {});
this.setPositionClasses(this.positionX, this.positionY);
this.setPositionClasses();
}

/** Event emitted when the menu is closed. */
Expand All @@ -115,29 +120,15 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
this.close.emit();
}

private _setPositionX(pos: MenuPositionX): void {
if (pos !== 'before' && pos !== 'after') {
throw new MdMenuInvalidPositionX();
}
this.positionX = pos;
}

private _setPositionY(pos: MenuPositionY): void {
if (pos !== 'above' && pos !== 'below') {
throw new MdMenuInvalidPositionY();
}
this.positionY = pos;
}

/**
* It's necessary to set position-based classes to ensure the menu panel animation
* folds out from the correct direction.
*/
setPositionClasses(posX: MenuPositionX, posY: MenuPositionY): void {
this._classList['mat-menu-before'] = posX == 'before';
this._classList['mat-menu-after'] = posX == 'after';
this._classList['mat-menu-above'] = posY == 'above';
this._classList['mat-menu-below'] = posY == 'below';
setPositionClasses(posX = this.xPosition, posY = this.yPosition): void {
this._classList['mat-menu-before'] = posX === 'before';
this._classList['mat-menu-after'] = posX === 'after';
this._classList['mat-menu-above'] = posY === 'above';
this._classList['mat-menu-below'] = posY === 'below';
}

}
12 changes: 6 additions & 6 deletions src/lib/menu/menu-errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,27 @@ export class MdMenuMissingError extends MdError {
}

/**
* Exception thrown when menu's x-position value isn't valid.
* Exception thrown when menu's xPosition value isn't valid.
* In other words, it doesn't match 'before' or 'after'.
* @docs-private
*/
export class MdMenuInvalidPositionX extends MdError {
constructor() {
super(`x-position value must be either 'before' or after'.
Example: <md-menu x-position="before" #menu="mdMenu"></md-menu>
super(`xPosition value must be either 'before' or after'.
Example: <md-menu xPosition="before" #menu="mdMenu"></md-menu>
`);
}
}

/**
* Exception thrown when menu's y-position value isn't valid.
* Exception thrown when menu's yPosition value isn't valid.
* In other words, it doesn't match 'above' or 'below'.
* @docs-private
*/
export class MdMenuInvalidPositionY extends MdError {
constructor() {
super(`y-position value must be either 'above' or below'.
Example: <md-menu y-position="above" #menu="mdMenu"></md-menu>
super(`yPosition value must be either 'above' or below'.
Example: <md-menu yPosition="above" #menu="mdMenu"></md-menu>
`);
}
}
4 changes: 2 additions & 2 deletions src/lib/menu/menu-panel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import {EventEmitter, TemplateRef} from '@angular/core';
import {MenuPositionX, MenuPositionY} from './menu-positions';

export interface MdMenuPanel {
positionX: MenuPositionX;
positionY: MenuPositionY;
xPosition: MenuPositionX;
yPosition: MenuPositionY;
overlapTrigger: boolean;
templateRef: TemplateRef<any>;
close: EventEmitter<void>;
Expand Down
4 changes: 2 additions & 2 deletions src/lib/menu/menu-trigger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,10 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
*/
private _getPosition(): ConnectedPositionStrategy {
const [posX, fallbackX]: HorizontalConnectionPos[] =
this.menu.positionX === 'before' ? ['end', 'start'] : ['start', 'end'];
this.menu.xPosition === 'before' ? ['end', 'start'] : ['start', 'end'];

const [overlayY, fallbackOverlayY]: VerticalConnectionPos[] =
this.menu.positionY === 'above' ? ['bottom', 'top'] : ['top', 'bottom'];
this.menu.yPosition === 'above' ? ['bottom', 'top'] : ['top', 'bottom'];

let originY = overlayY;
let fallbackOriginY = fallbackOverlayY;
Expand Down
48 changes: 39 additions & 9 deletions src/lib/menu/menu.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,13 @@ describe('MdMenu', () => {
});

describe('positions', () => {
let fixture: ComponentFixture<PositionedMenu>;
let panel: HTMLElement;

beforeEach(() => {
const fixture = TestBed.createComponent(PositionedMenu);
fixture = TestBed.createComponent(PositionedMenu);
fixture.detectChanges();

const trigger = fixture.componentInstance.triggerEl.nativeElement;

// Push trigger to the bottom edge of viewport,so it has space to open "above"
Expand All @@ -123,18 +126,43 @@ describe('MdMenu', () => {

fixture.componentInstance.trigger.openMenu();
fixture.detectChanges();
panel = overlayContainerElement.querySelector('.mat-menu-panel') as HTMLElement;
});

it('should append mat-menu-before if x position is changed', () => {
const panel = overlayContainerElement.querySelector('.mat-menu-panel');
it('should append mat-menu-before if the x position is changed', () => {
expect(panel.classList).toContain('mat-menu-before');
expect(panel.classList).not.toContain('mat-menu-after');

fixture.componentInstance.xPosition = 'after';
fixture.detectChanges();

expect(panel.classList).toContain('mat-menu-after');
expect(panel.classList).not.toContain('mat-menu-before');
});

it('should append mat-menu-above if y position is changed', () => {
const panel = overlayContainerElement.querySelector('.mat-menu-panel');
it('should append mat-menu-above if the y position is changed', () => {
expect(panel.classList).toContain('mat-menu-above');
expect(panel.classList).not.toContain('mat-menu-below');

fixture.componentInstance.yPosition = 'below';
fixture.detectChanges();

expect(panel.classList).toContain('mat-menu-below');
expect(panel.classList).not.toContain('mat-menu-above');
});

it('should default to the "below" and "after" positions', () => {
fixture.destroy();

let newFixture = TestBed.createComponent(SimpleMenu);

newFixture.detectChanges();
newFixture.componentInstance.trigger.openMenu();
newFixture.detectChanges();
panel = overlayContainerElement.querySelector('.mat-menu-panel') as HTMLElement;

expect(panel.classList).toContain('mat-menu-below');
expect(panel.classList).toContain('mat-menu-after');
});

});
Expand Down Expand Up @@ -193,7 +221,7 @@ describe('MdMenu', () => {
.toBe(Math.round(expectedTop),
`Expected menu to open in "above" position if "below" position wouldn't fit.`);

// The x-position of the overlay should be unaffected, as it can already fit horizontally
// The xPosition of the overlay should be unaffected, as it can already fit horizontally
expect(Math.round(overlayRect.left))
.toBe(Math.round(triggerRect.left),
`Expected menu x position to be unchanged if it can fit in the viewport.`);
Expand Down Expand Up @@ -437,14 +465,16 @@ class SimpleMenu {
@Component({
template: `
<button [mdMenuTriggerFor]="menu" #triggerEl>Toggle menu</button>
<md-menu x-position="before" y-position="above" #menu="mdMenu">
<md-menu [xPosition]="xPosition" [yPosition]="yPosition" #menu="mdMenu">
<button md-menu-item> Positioned Content </button>
</md-menu>
`
})
class PositionedMenu {
@ViewChild(MdMenuTrigger) trigger: MdMenuTrigger;
@ViewChild('triggerEl') triggerEl: ElementRef;
xPosition: MenuPositionX = 'before';
yPosition: MenuPositionY = 'above';
}

interface TestableMenu {
Expand Down Expand Up @@ -476,8 +506,8 @@ class OverlapMenu implements TestableMenu {
exportAs: 'mdCustomMenu'
})
class CustomMenuPanel implements MdMenuPanel {
positionX: MenuPositionX = 'after';
positionY: MenuPositionY = 'below';
xPosition: MenuPositionX = 'after';
yPosition: MenuPositionY = 'below';
overlapTrigger: true;

@ViewChild(TemplateRef) templateRef: TemplateRef<any>;
Expand Down

0 comments on commit 1fd50aa

Please sign in to comment.