feat(menu): Added ability to show the menu overlay around the menu trigger #1771

trshafer · 2016-11-08T18:09:27Z

Currently the menu is fixed to show over the menu trigger.

This adds a (rather blunt) boolean to decide if the menu should be shown above or below the trigger. I thought about changing the possibilities to overlay-position: default | above | below | left | right; But that felt like a bit too much flexibility. I think some version of this feature is really important for those designing their own menu. Let me know what you think.

Thanks,
Thomas

Here is the menu below the trigger:

Here is the menu above the trigger:

jelbourn · 2016-11-16T01:25:59Z

src/lib/menu/README.md

-using the `x-position` (`before | after`) and `y-position` (`above | below`) attributes.  
+By default, the menu will display after and below its trigger.  You can change this display position
+using the `x-position` (`before | after`) and `y-position` (`above | below`) attributes. The menu
+can be positioned over the menu button or outside using `overlay-trigger` (`true | false`).


I'm not a fan of this name ("overlay-trigger")

@kara thoughts?

I totally agree. It's also not very flexible. Happy to move it to something other than a boolean, perhaps "cover", "horizontal", and "vertical"? I dunno, those are pretty bad too.

How about "overlap-trigger"? "Overlay-trigger" might sound like the trigger itself is an overlay.

jelbourn · 2016-11-16T01:26:10Z

R: @kara

kara

Can you add tests for this?

kara · 2017-01-03T19:08:58Z

src/lib/menu/menu-directive.ts


  @ViewChild(TemplateRef) templateRef: TemplateRef<any>;
  @ContentChildren(MdMenuItem) items: QueryList<MdMenuItem>;

  constructor(@Attribute('x-position') posX: MenuPositionX,
-              @Attribute('y-position') posY: MenuPositionY) {
+              @Attribute('y-position') posY: MenuPositionY,
+              @Attribute('overlay-trigger') trigger = true) {


Have you considered using the public keyword instead? That way you don't have to explicitly set it again on line 60.

@Attribute('overlap-trigger') public overlapTrigger = true

Thanks for the suggestion.

kara · 2017-01-03T19:11:47Z

src/lib/menu/README.md

@@ -148,7 +149,8 @@ also adds `aria-hasPopup="true"` to the trigger element.
 | --- | --- | --- |
 | `x-position` | `before | after` | The horizontal position of the menu in relation to the trigger. Defaults to `after`. | 
 | `y-position` | `above | below` | The vertical position of the menu in relation to the trigger. Defaults to `below`. |
-
+| `overlay-trigger`    | `true | false` | Whether to have the menu show on top of the menu trigger or outside. Defaults to `true`. |


Can we rename to overlapTrigger?

kara · 2017-01-03T19:22:00Z

src/lib/menu/menu-trigger.ts

+    let originY = overlayY;
+    if (this.menu.overlayTrigger) {
+      originY = overlayY === 'top' ? 'bottom' : 'top';
+    }


If you want to overlap the trigger, shouldn't the originY and the overlayY be the same? e.g. the top of the trigger should connect to the top of the overlay or the bottom of the trigger should connect to the bottom of the overlay. Let me know if I'm missing something.

let originY: VerticalConnectionPos; if (this.menu.overlapTrigger) { originY = overlayY; } else { originY = overlayY === 'top' ? 'bottom' : 'top'; }

One would think so. However I believe the current implementation produces the desired effect. Are you able to clone the pr and run the demo-app and verify? I have done so and I believe the functionality is accurate.

trshafer · 2017-01-04T23:52:50Z

Not sure why it says the checks have passed, the tests are failing on my local machine. I'm working on it.

trshafer · 2017-01-04T23:58:10Z

Ah, looks like there's some test bleedover. If I just focus on the fallback positions describe block using fdescribe, those tests will fail. If I focus on the whole menu test then the fallback positions test passes. Tested this on master.

trshafer · 2017-01-05T00:05:50Z

I have logged the issue: #2530.

trshafer · 2017-01-05T00:10:58Z

Hey @kara, I'm happy to write tests for the updated change. However since the tests are currently failing I don't feel confident in updating the tests or making new ones without potentially causing issues. Are you able to update the tests? I could attempt to fix the tests, however since I don't have the background knowledge in those tests, I thought it would be better to defer to the original author. Looks like they were introduced here: 7572e34

kara · 2017-01-05T00:15:43Z

@trshafer We won't be able to merge this PR without tests; it's standard practice for this repo to provide tests with your code. Go ahead and give it a shot. I can take a look at the other issue you found in the meantime.

trshafer · 2017-01-05T00:20:18Z

Totally. Happy to write tests. I should have red, green, refactored my way to this PR. Starting from a red test is harder for me to confidently ensure the test and change I have written is correct. It would be great if the menu test suite was green before I start.

I've also rebased in the recent commit.

kara · 2017-01-05T00:21:52Z

Yep, that makes sense. I'll ping you once I've looked at the the existing tests.

trshafer · 2017-01-09T22:16:47Z

Hey @kara, thanks for submitting the test fixes. I have written two tests for the overlap change.

I made an OverlapSubject class to reduce the testing duplication. Currently its visibility is limited to the new test describe block, however it might be helpful for the other tests as well.

kara · 2017-01-12T02:11:39Z

src/lib/menu/README.md

@@ -115,7 +115,8 @@ Output:
 ### Customizing menu position

 By default, the menu will display after and below its trigger.  You can change this display position
-using the `x-position` (`before | after`) and `y-position` (`above | below`) attributes.
+using the `x-position` (`before | after`) and `y-position` (`above | below`) attributes. The menu
+can be positioned over the menu button or outside using `overlap-trigger` (`true | false`).


Can you update these in the OVERVIEW as well? The docs are generated from those files now, and the READMEs are probably going away soon.

Updated. Let me know if there is more documentation I should add.

kara · 2017-01-12T02:16:12Z

src/lib/menu/menu.spec.ts

+      }
+
+      updateTriggerStyle(style: any) {
+        return Object.assign(this.trigger.style, style);


Can you use extendObject from core instead?

kara · 2017-01-12T02:18:38Z

src/lib/menu/menu.spec.ts

+  `
+})
+class NonOverlapMenu implements TestableMenu {
+  @ViewChild(MdMenuTrigger) trigger: MdMenuTrigger;


It doesn't look like you're using this. Remove?

It's being used to open the menu: https://github.com/trshafer/material2/blob/5373c66e8cd9d0a06485158434d0afbc5233245a/src/lib/menu/menu.spec.ts#L271

I see. Totally missed that!

kara · 2017-01-12T02:21:01Z

src/lib/menu/menu.spec.ts

+      // 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 in "above" position if "below" position wouldn't fit.`);


Copy paste error? I think it should be "Expected menu to open in "below" position by default."

kara · 2017-01-12T02:26:44Z

src/lib/menu/menu.spec.ts

@@ -256,6 +256,66 @@ describe('MdMenu', () => {
    }
  });

+  describe('not overlapping', () => {
+    class OverlapSubject<T extends TestableMenu> {


Add comment describing class?

kara · 2017-01-12T02:34:17Z

src/lib/menu/menu-directive.ts

@@ -54,7 +54,8 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
  @ContentChildren(MdMenuItem) items: QueryList<MdMenuItem>;

  constructor(@Attribute('x-position') posX: MenuPositionX,
-              @Attribute('y-position') posY: MenuPositionY) {
+              @Attribute('y-position') posY: MenuPositionY,
+              @Attribute('overlap-trigger') public overlapTrigger = true) {
    if (posX) { this._setPositionX(posX); }
    if (posY) { this._setPositionY(posY); }


You'll need to update the positionY when the trigger is not overlapping, otherwise the animation origin will be the wrong side of the menu. If you slow the animation down to 10%, you'll see it's animating from the wrong direction.

Good catch. I have updated the menu positioning classes and added a test.

kara · 2017-01-12T02:46:05Z

src/lib/menu/menu-trigger.ts

+
+    if (this.menu.overlapTrigger) {
+      originY = overlayY === 'top' ? 'bottom' : 'top';
+      fallbackOriginY = fallbackOverlayY === 'top' ? 'bottom' : 'top';


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 overlapTrigger is a string. Strings are truthy, so if you set overlapTrigger to anything (including "false"), this.menu.overlapTrigger will be truthy and execute this overlap block. When you don't set the attribute (like in the original tests/demos), this.menu.overlapTrigger is null, so it doesn't default to true and doesn't hit this condition. If you set overlapTrigger="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.

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.

kara · 2017-01-12T02:46:39Z

src/lib/menu/menu.spec.ts

+          .toBe(Math.round(subject.triggerRect.top),
+              `Expected menu to open in "above" position if "below" position wouldn't fit.`);
+    });
+  });


Can you add a test for when overlapTrigger="true"?

trshafer · 2017-01-18T23:06:03Z

Hello @kara, thank you for the thorough review. I'm pretty sure I have responded to all of your comments. Let me know what else I can do to help move this across the finish line.

kara

LGTM

mmalerba

When I try to merge this into Google's codebase I get the following error:
Class 'Menu' incorrectly implements interface 'MdMenuPanel'.
Property 'overlapTrigger' is missing in type 'Menu'.

mmalerba · 2017-01-20T17:48:02Z

src/lib/menu/menu-directive.ts

@@ -52,6 +52,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {

  @ViewChild(TemplateRef) templateRef: TemplateRef<any>;
  @ContentChildren(MdMenuItem) items: QueryList<MdMenuItem>;
+  @Input('overlap-trigger') overlapTrigger = true;


Don't need to rename to dash-case, we're trying to switch everything to camelCase

so should the api look like <md-menu [overlapTrigger]="false">?

trshafer · 2017-01-20T18:42:50Z

Switched overlap-trigger to overlapTrigger.

mmalerba · 2017-01-20T18:56:47Z

src/lib/menu/menu-directive.ts

@@ -52,6 +52,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {

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


tiny nit: if the variable and the html attr have the same name you can omit the alias:
@Input() overlapTrigger = true;

Done, here and in the test.

…igger

angular-automatic-lock-bot · 2019-09-06T07:02:17Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 8, 2016

trshafer force-pushed the menu-positioning branch 2 times, most recently from 56de2a8 to 72979c7 Compare November 11, 2016 17:14

jelbourn assigned kara Nov 12, 2016

jelbourn added the pr: needs review label Nov 12, 2016

jelbourn reviewed Nov 16, 2016

View reviewed changes

kara requested changes Jan 3, 2017

View reviewed changes

kara added pr: needs rebase and removed pr: needs review labels Jan 3, 2017

trshafer force-pushed the menu-positioning branch from 72979c7 to 8bfb6c5 Compare January 4, 2017 23:44

trshafer force-pushed the menu-positioning branch 3 times, most recently from 659775a to 5373c66 Compare January 9, 2017 22:14

kara requested changes Jan 12, 2017

View reviewed changes

kara removed the pr: needs rebase label Jan 12, 2017

kara removed their assignment Jan 17, 2017

trshafer force-pushed the menu-positioning branch 4 times, most recently from b855e4f to ea72a57 Compare January 18, 2017 22:56

trshafer force-pushed the menu-positioning branch from ea72a57 to b21bf85 Compare January 18, 2017 22:56

kara added pr: needs review and removed pr: needs cleanup / tests labels Jan 19, 2017

kara approved these changes Jan 19, 2017

View reviewed changes

kara added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 19, 2017

mmalerba reviewed Jan 20, 2017

View reviewed changes

trshafer force-pushed the menu-positioning branch 2 times, most recently from 26234eb to b8d6a51 Compare January 20, 2017 18:40

mmalerba reviewed Jan 20, 2017

View reviewed changes

trshafer force-pushed the menu-positioning branch from b8d6a51 to 17fc76e Compare January 20, 2017 19:07

feat(menu): Added ability to show the menu overlay around the menu tr…

3e12b1a

…igger

trshafer force-pushed the menu-positioning branch from 17fc76e to 3e12b1a Compare January 20, 2017 19:08

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed in progress This issue is currently in progress labels Jan 20, 2017

mmalerba merged commit 592f33f into angular:master Jan 20, 2017

trshafer deleted the menu-positioning branch January 22, 2017 01:38

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

feat(menu): Added ability to show the menu overlay around the menu trigger #1771

feat(menu): Added ability to show the menu overlay around the menu trigger #1771

Conversation

trshafer commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Nov 16, 2016

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trshafer commented Jan 4, 2017

trshafer commented Jan 4, 2017 • edited Loading

trshafer commented Jan 5, 2017

trshafer commented Jan 5, 2017

kara commented Jan 5, 2017 • edited Loading

trshafer commented Jan 5, 2017

kara commented Jan 5, 2017

trshafer commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trshafer commented Jan 18, 2017

kara left a comment

Choose a reason for hiding this comment

mmalerba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trshafer commented Jan 20, 2017

mmalerba Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

trshafer Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 6, 2019

trshafer commented Jan 4, 2017 •

edited

Loading

kara commented Jan 5, 2017 •

edited

Loading

kara Jan 12, 2017 •

edited

Loading

mmalerba Jan 20, 2017 •

edited

Loading

trshafer Jan 20, 2017 •

edited

Loading