feat(menu): Added ability to show the menu overlay around the menu trigger #1771
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| import {TestBed, async, ComponentFixture} from '@angular/core/testing'; | ||
| import {By} from '@angular/platform-browser'; | ||
| import { | ||
| Component, | ||
| ElementRef, | ||
| EventEmitter, | ||
| Input, | ||
| Output, | ||
| TemplateRef, | ||
| ViewChild | ||
|
|
@@ -18,6 +19,7 @@ import { | |
| import {OverlayContainer} from '../core/overlay/overlay-container'; | ||
| import {ViewportRuler} from '../core/overlay/position/viewport-ruler'; | ||
| import {Dir, LayoutDirection} from '../core/rtl/dir'; | ||
| import {extendObject} from '../core/util/object-extend'; | ||
|
|
||
| describe('MdMenu', () => { | ||
| let overlayContainerElement: HTMLElement; | ||
|
|
@@ -27,7 +29,7 @@ describe('MdMenu', () => { | |
| dir = 'ltr'; | ||
| TestBed.configureTestingModule({ | ||
| imports: [MdMenuModule.forRoot()], | ||
| declarations: [SimpleMenu, PositionedMenu, OverlapMenu, CustomMenuPanel, CustomMenu], | ||
| providers: [ | ||
| {provide: OverlayContainer, useFactory: () => { | ||
| overlayContainerElement = document.createElement('div'); | ||
|
|
@@ -256,6 +258,106 @@ describe('MdMenu', () => { | |
| } | ||
| }); | ||
|
|
||
| describe('overlapping trigger', () => { | ||
| /** | ||
| * This test class is used to create components containing a menu. | ||
| * It provides helpers to reposition the trigger, open the menu, | ||
| * and access the trigger and overlay positions. | ||
| * Additionally it can take any inputs for the menu wrapper component. | ||
| * | ||
| * Basic usage: | ||
| * const subject = new OverlapSubject(MyComponent); | ||
| * subject.openMenu(); | ||
| */ | ||
| class OverlapSubject<T extends TestableMenu> { | ||
|
There was a problem hiding this comment. Add comment describing class? |
||
| private readonly fixture: ComponentFixture<T>; | ||
| private readonly trigger: any; | ||
|
|
||
| constructor(ctor: {new(): T; }, inputs: {[key: string]: any} = {}) { | ||
| this.fixture = TestBed.createComponent(ctor); | ||
| extendObject(this.fixture.componentInstance, inputs); | ||
| this.fixture.detectChanges(); | ||
| this.trigger = this.fixture.componentInstance.triggerEl.nativeElement; | ||
| } | ||
|
|
||
| openMenu() { | ||
| this.fixture.componentInstance.trigger.openMenu(); | ||
| this.fixture.detectChanges(); | ||
| } | ||
|
|
||
| updateTriggerStyle(style: any) { | ||
| return extendObject(this.trigger.style, style); | ||
| } | ||
|
|
||
| get overlayRect() { | ||
| return this.overlayPane.getBoundingClientRect(); | ||
| } | ||
|
|
||
| get triggerRect() { | ||
| return this.trigger.getBoundingClientRect(); | ||
| } | ||
|
|
||
| get menuPanel() { | ||
| return overlayContainerElement.querySelector('.md-menu-panel'); | ||
| } | ||
|
|
||
| private get overlayPane() { | ||
| return overlayContainerElement.querySelector('.cdk-overlay-pane') as HTMLElement; | ||
| } | ||
| } | ||
|
|
||
| let subject: OverlapSubject<OverlapMenu>; | ||
| describe('explicitly overlapping', () => { | ||
| beforeEach(() => { | ||
| subject = new OverlapSubject(OverlapMenu, {overlapTrigger: true}); | ||
| }); | ||
|
|
||
| it('positions the overlay below the trigger', () => { | ||
| subject.openMenu(); | ||
|
|
||
| // Since the menu is overlaying the trigger, the overlay top should be the trigger top. | ||
| expect(Math.round(subject.overlayRect.top)) | ||
| .toBe(Math.round(subject.triggerRect.top), | ||
| `Expected menu to open in default "below" position.`); | ||
| }); | ||
| }); | ||
|
|
||
| describe('not overlapping', () => { | ||
| beforeEach(() => { | ||
| subject = new OverlapSubject(OverlapMenu, {overlapTrigger: false}); | ||
| }); | ||
|
|
||
| it('positions the overlay below the trigger', () => { | ||
| subject.openMenu(); | ||
|
|
||
| // Since the menu is below the trigger, the overlay top should be the trigger bottom. | ||
| expect(Math.round(subject.overlayRect.top)) | ||
| .toBe(Math.round(subject.triggerRect.bottom), | ||
| `Expected menu to open directly below the trigger.`); | ||
| }); | ||
|
|
||
| it('supports above position fall back', () => { | ||
| // Push trigger to the bottom part of viewport, so it doesn't have space to open | ||
| // in its default "below" position below the trigger. | ||
| subject.updateTriggerStyle({position: 'relative', top: '650px'}); | ||
| subject.openMenu(); | ||
|
|
||
| // Since the menu is above the trigger, the overlay bottom should be the trigger top. | ||
| expect(Math.round(subject.overlayRect.bottom)) | ||
| .toBe(Math.round(subject.triggerRect.top), | ||
| `Expected menu to open in "above" position if "below" position wouldn't fit.`); | ||
| }); | ||
|
|
||
| it('repositions the origin to be below, so the menu opens from the trigger', () => { | ||
| subject.openMenu(); | ||
|
|
||
| expect(subject.menuPanel.classList).toContain('md-menu-below'); | ||
| expect(subject.menuPanel.classList).not.toContain('md-menu-above'); | ||
| }); | ||
|
|
||
| }); | ||
| }); | ||
|
There was a problem hiding this comment. Can you add a test for when |
||
|
|
||
| describe('animations', () => { | ||
| it('should include the ripple on items by default', () => { | ||
| const fixture = TestBed.createComponent(SimpleMenu); | ||
|
|
@@ -311,6 +413,23 @@ class PositionedMenu { | |
| @ViewChild('triggerEl') triggerEl: ElementRef; | ||
| } | ||
|
|
||
| interface TestableMenu { | ||
| trigger: MdMenuTrigger; | ||
| triggerEl: ElementRef; | ||
| } | ||
| @Component({ | ||
| template: ` | ||
| <button [mdMenuTriggerFor]="menu" #triggerEl>Toggle menu</button> | ||
| <md-menu [overlapTrigger]="overlapTrigger" #menu="mdMenu"> | ||
| <button md-menu-item> Not overlapped Content </button> | ||
| </md-menu> | ||
| ` | ||
| }) | ||
| class OverlapMenu implements TestableMenu { | ||
| @Input() overlapTrigger: boolean; | ||
| @ViewChild(MdMenuTrigger) trigger: MdMenuTrigger; | ||
|
There was a problem hiding this comment. It doesn't look like you're using this. Remove? There was a problem hiding this comment. It's being used to open the menu: https://github.com/trshafer/material2/blob/5373c66e8cd9d0a06485158434d0afbc5233245a/src/lib/menu/menu.spec.ts#L271 |
||
| @ViewChild('triggerEl') triggerEl: ElementRef; | ||
| } | ||
|
|
||
| @Component({ | ||
| selector: 'custom-menu', | ||
|
|
@@ -325,6 +444,7 @@ class PositionedMenu { | |
| class CustomMenuPanel implements MdMenuPanel { | ||
| positionX: MenuPositionX = 'after'; | ||
| positionY: MenuPositionY = 'below'; | ||
| overlapTrigger: true; | ||
|
|
||
| @ViewChild(TemplateRef) templateRef: TemplateRef<any>; | ||
| @Output() close = new EventEmitter<void>(); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning to our previous discussion, this logic is actually backwards. The originY should match the overlayY when the menu is overlapping. When it's not overlapping, the top of the overlay should hit the bottom of the trigger, etc.
The reason that this happens to work in the demo and the tests is because
overlapTriggeris a string. Strings are truthy, so if you setoverlapTriggerto anything (including "false"),this.menu.overlapTriggerwill be truthy and execute this overlap block. When you don't set the attribute (like in the original tests/demos),this.menu.overlapTriggeris null, so it doesn't default to true and doesn't hit this condition. If you setoverlapTrigger="true"in the demo, you'll see that all values for overlapTrigger will make the menu not overlap, including "true".I'd coerce this.menu.overlapTrigger to a boolean in menu-directive.ts, and invert the logic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely correct. The test of an explicit overlapTrigger = true is a good suggestion and exposes this in a test. Thanks for pushing back. Fixed.