feat(menu): Adds new menu, menu-surface. #3311

williamernest · 2018-08-07T23:58:05Z

Resolves #2972, resolves #2770, resolves #2763, resolves #2758, resolves #2756, resolves #2691, resolves #2288, resolves #1845, resolves #1495

Adds the ability for a menu-surface to be positioned relative to an element, relative to the page, or fixed to the viewport. Adds the ability to dynamically change the anchoring element. Adds the ability to hoist the menu to the body.

BREAKING CHANGE: Menu positioning logic has been split into its own package (mdc-menu-surface). mdc-menu is rebuilt to use mdc-menu-surface and mdc-list styles and JavaScript.

Adds the ability to change the anchor element of a menu surface dynamically. Also adds a function to allow the user to hoist the menu to the body, and calculates the position of the menu in both absolute and fixed positioning.

)

This reverts commit 01f6b33.

mdc-web-bot · 2018-08-08T00:05:33Z

All 188 screenshot tests passed for commit 01e071a vs. master! 💯🎉

codecov-io · 2018-08-08T00:08:40Z

Codecov Report

Merging #3311 into master will increase coverage by 0.08%.
The diff coverage is 98.77%.

@@            Coverage Diff             @@
##           master    #3311      +/-   ##
==========================================
+ Coverage   98.05%   98.13%   +0.08%     
==========================================
  Files         120      123       +3     
  Lines        5141     5215      +74     
  Branches      643      638       -5     
==========================================
+ Hits         5041     5118      +77     
+ Misses        100       97       -3

Impacted Files	Coverage Δ
packages/mdc-menu/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-list/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-menu/foundation.js	`100% <100%> (+0.97%)`	⬆️
packages/mdc-menu-surface/util.js	`100% <100%> (ø)`
packages/mdc-menu-surface/constants.js	`100% <100%> (ø)`
packages/mdc-menu/index.js	`100% <100%> (+6.09%)`	⬆️
packages/mdc-menu-surface/foundation.js	`97.8% <97.8%> (ø)`
packages/mdc-menu-surface/index.js	`99.02% <99.02%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5160241...92ec26b. Read the comment docs.

mdc-web-bot · 2018-08-08T00:08:45Z

All 188 screenshot tests passed for commit d55bdc5 vs. master! 💯🎉

kfranqueiro

I did a review of menu-surface and paused there. I kinda had a lot of questions and some of them are probably just due to lack of context.

kfranqueiro · 2018-08-08T20:13:45Z

demos/menu.html

+
+        menuEl4.addEventListener('MDCMenu:selected', function(evt) {
+          var detail = evt.detail;
+          lastSelected.textContent = 'Overflow Menu: "' + detail.item.textContent.trim() +


Overflow -> Context

kfranqueiro · 2018-08-08T20:14:14Z

demos/menu.html

-        remember.addEventListener('change', function(evt) {
-          menu.rememberSelection = evt.target.checked;
+        // Normal menu
+        var menuEl2 = document.querySelector('#demo-menu-2');


Nit: The rest of these are using getElementById

kfranqueiro · 2018-08-08T20:14:55Z

demos/menu.scss

+html,
+body,
+main {
+  position: relative;


Are both of these styles actually needed/useful on all of these elements? (in both demo pages)

position: relative is required for body, but I removed the rest.

kfranqueiro · 2018-08-08T20:24:41Z

packages/mdc-menu-surface/README.md

+`isOpen() => boolean` | Returns a boolean indicating whether the menu surface is open.
+`setQuickOpen(quickOpen: boolean) => void` | Sets whether the menu surface should open and close without animation when the `open`/`close` methods are called.
+
+### Events


Should this be moved to under the component properties and methods?

kfranqueiro · 2018-08-08T20:26:35Z

packages/mdc-menu-surface/README.md

+
+Method Signature | Description
+--- | ---
+`show() => void` | Proxies to the foundation's `open()` method.


Why are these named differently between the component and foundation? I realize we can't call them open/close on the component, but then should we switch to show/hide on the foundation, too?

Removed show/hide

kfranqueiro · 2018-08-08T21:27:29Z

packages/mdc-menu-surface/mdc-menu-surface.scss

+@import "@material/rtl/mixins";
+@import "./mixins";
+
+$mdc-menu-surface-fade-in-duration: .03s;


-> _variables.scss?

kfranqueiro · 2018-08-08T21:29:59Z

packages/mdc-menu-surface/mdc-menu-surface.scss

+  transform-origin: top left;
+  border-radius: 4px;
+  opacity: 0;
+  white-space: nowrap;


Does this make sense on menu-surface? Seems ultimately pertinent to #3201, maybe, but maybe not right here?

kfranqueiro · 2018-08-08T21:30:25Z

packages/mdc-menu-surface/mdc-menu-surface.scss

+  display: none;
+  position: absolute;
+  box-sizing: border-box;
+  max-width: calc(100vw - 32px);


Should 32px be a variable?

kfranqueiro · 2018-08-08T21:31:21Z

packages/mdc-menu-surface/mdc-menu-surface.scss

+  padding: 0;
+  transform: scale(1);
+  transform-origin: top left;
+  border-radius: 4px;


Should this have a corner-radius mixin?

kfranqueiro · 2018-08-08T21:32:44Z

packages/mdc-menu-surface/package.json

+    "url": "https://github.com/material-components/material-components-web.git"
+  },
+  "dependencies": {
+    "@material/animation": "^0.34.0",


Are these versions up to date?

mdc-web-bot · 2018-08-08T23:51:29Z

All 188 screenshot tests passed for commit 3f45dd1 vs. master! 💯🎉

mdc-web-bot · 2018-08-09T00:45:32Z

All 188 screenshot tests passed for commit 522a8a8 vs. master! 💯🎉

mdc-web-bot · 2018-08-09T00:58:14Z

All 188 screenshot tests passed for commit 6d6ccd9 vs. master! 💯🎉

kfranqueiro · 2018-08-09T17:08:13Z

packages/mdc-menu-surface/README.md

+#### Absolute Position
+
+The menu surface can use absolute positioning when being displayed. This requires that the element containing the 
+menu(`body` if using `hoistMenyToBody()`) has the `position: relative` style. 


Meny -> Menu

Add space before (

kfranqueiro · 2018-08-09T17:21:12Z

packages/mdc-menu-surface/foundation.js

+    if (isRightAligned) {
+      const rightOffset = avoidHorizontalOverlap ? anchorWidth - this.anchorMargin_.left : this.anchorMargin_.right;
+
+      // Hoisted elements positioning doesn't account for the scrollbar, so the right property needs to be reduced by


I'm a little confused, since AFAICT the code still looks like it's doing something related to factoring out the scrollbar and only when isRightAligned is true. So how is the comment outdated?

kfranqueiro · 2018-08-09T17:50:05Z

packages/mdc-menu-surface/README.md

+</div>
+```
+
+```js


There should probably be a "Or in JS:" before this, right? You only need one or the other IIUC?

kfranqueiro · 2018-08-09T17:56:38Z

packages/mdc-menu/index.js

+    this.menuSurface_.anchorElement = element;
+  }
+
+  registerListeners_() {


I'm curious why we are registering and deregistering listeners based on when it's open/closed. Is there any harm to just having them hooked up all the time? (It'd be one thing if they were registered on the body, but these are on the menu surface element.)

kfranqueiro · 2018-08-09T17:57:52Z

packages/mdc-menu/index.js

  }

  /**
-   * Return the item within the menu that is selected.
+   * Return the item within the menu at the index specified.


Do we foresee a purpose for this API? I know it existed before but I'm not sure why.

I believe this was here previously so we could attempt to replicate .item(index) functionality from the native select. I planned on implementing this in select so I included it here, but it may not be required.

kfranqueiro · 2018-08-09T20:21:08Z

packages/mdc-menu-surface/util.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016 Google Inc. All Rights Reserved.
+ * Copyright 2018 Google Inc. All Rights Reserved.


While we're here, I think there are a bunch of these held over from old JS animation logic in "Simple" Menu that we don't need anymore (they're only referenced in tests):

clamp

bezierProgress (and getBezierCoordinate_ and solvePositionFromXValue_)

kfranqueiro · 2018-08-09T20:22:31Z

packages/mdc-menu/README.md

-`items` | Array<Element> | Proxies to the foundation's container to query for all `.mdc-list-item[role]` elements.
-`itemsContainer` | Element | Queries the foundation's root element for the `mdc-menu__items` container element.
-`quickOpen` | Boolean | Proxies to the foundation's `setQuickOpen()` method.
+`open` | Boolean | `get` proxies to the menu surface's `open` property. `set` proxies to the `show` and `hide`methods.


Missing space before "methods"

kfranqueiro · 2018-08-09T20:23:33Z

packages/mdc-menu/README.md


 Method Signature | Description
 --- | ---
-`show({focusIndex: ?number}) => void` | Proxies to the foundation's `open()` method. An optional config parameter allows the caller to specify which item should receive focus after the menu opens.
+`show() => void` | Proxies to the menu surface's `open()` method.


show and hide were removed, right?

kfranqueiro · 2018-08-09T20:46:29Z

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

+  td.verify(mockFoundation.setIsHoisted(true));
+});
+
+test('setFixedPosition is false', () => {


Move this up below the setFixedPosition is true test?

kfranqueiro · 2018-08-09T20:46:58Z

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

+  td.verify(mockFoundation.setQuickOpen(false));
+});
+
+test('foundationAdapter#addClass adds a class to the root element', () => {


foundationAdapter -> adapter

kfranqueiro · 2018-08-09T20:48:42Z

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

+  document.body.removeChild(root);
+});
+
+test('adapter#restoreFocus does not restores the focus if never called adapter#saveFocus', () => {


restores -> restore

kfranqueiro · 2018-08-09T20:55:57Z

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

+  const {root, component} = setupTest(true);
+  document.body.appendChild(root);
+  component.initialSyncWithDOM();
+  assert.equal(component.getDefaultFoundation().adapter_.getAnchorDimensions(), undefined);


assert.isUndefined

kfranqueiro · 2018-08-09T22:51:50Z

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

+  td.verify(handler(td.matchers.anything()));
+});
+
+test('adapter#restoreFocus restore focus saved by adapter#saveFocus', () => {


This one was correct with "restores" :|

kfranqueiro · 2018-08-09T22:52:00Z

packages/mdc-menu-surface/util.js

+
+  return storedTransformPropertyName_;
+}
+export {getTransformPropertyName};


Add blank line before export

kfranqueiro · 2018-08-09T22:52:27Z

packages/mdc-menu-surface/adapter.js

+   * @param {string} type
+   * @param {function(!Event)} handler
+   */
+  registerInteractionHandler(type, handler) {}


Do we want to move keydown registration to the component now or in a separate PR?

kfranqueiro · 2018-08-09T22:54:23Z

packages/mdc-menu-surface/foundation.js

+   * types.
+   * @return {!MDCMenuSurfaceAdapter}
+   */
+  static get defaultAdapter() {


default adapter is missing focus... either that or focus shouldn't be in adapter, and readme?

mdc-web-bot · 2018-08-10T17:58:43Z

All 344 screenshot tests passed for commit 99fa8f9 vs. master! 💯🎉

kfranqueiro · 2018-08-10T18:14:28Z

packages/mdc-menu-surface/index.js

+  destroy() {
+    this.root_.removeEventListener('keydown', this.handleKeydown_);
+    this.root_.removeEventListener(strings.OPENED_EVENT, this.registerBodyClickListener_);
+    this.root_.removeEventListener(strings.CLOSED_EVENT, this.deregisterBodyClickListener_);


Don't forget to call super.destroy()

kfranqueiro · 2018-08-10T18:17:53Z

packages/mdc-menu/index.js

+    this.handleClick_ = (evt) => this.foundation_.handleClick(evt);
+
+    this.menuSurface_.listen(MDCMenuSurfaceFoundation.strings.OPENED_EVENT, this.afterOpenedCallback_);
+    this.registerListeners_();


Is there any point to [de]registerListeners existing anymore or should we just move them here (and use [un]listen for their registrations)?

kfranqueiro · 2018-08-10T18:19:32Z

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

+  assert.equal(component.getDefaultFoundation().adapter_.getInnerDimensions().height, root.offsetHeight);
+});
+
+//


Remove (I commented on the individual commit before, oops)

kfranqueiro · 2018-08-10T18:23:00Z

test/unit/mdc-menu/mdc-menu.test.js

-  component.setAnchorCorner(Corner.TOP_START);
-  // The method sets private variable on the foundation, nothing to verify.
-});
+test('destory deregisters event listener for click', () => {


destory -> destroy

kfranqueiro · 2018-08-10T18:23:06Z

test/unit/mdc-menu/mdc-menu.test.js

-  const {component} = setupTest();
-  assert.isOk(!component.selectedItem);
-});
+test('destory deregisters event listener for keydown', () => {


destory -> destroy

mdc-web-bot · 2018-08-10T18:32:02Z

All 344 screenshot tests passed for commit 08a9ccc vs. master! 💯🎉

mdc-web-bot · 2018-08-13T19:28:55Z

All 337 screenshot tests passed for commit 5202b7c vs. master! 💯🎉

mdc-web-bot · 2018-08-13T19:39:49Z

All 337 screenshot tests passed for commit c9d198f vs. master! 💯🎉

mdc-web-bot · 2018-08-13T19:41:34Z

All 337 screenshot tests passed for commit 87260ed vs. master! 💯🎉

mdc-web-bot · 2018-08-13T22:35:51Z

All 337 screenshot tests passed for commit 92ec26b vs. master! 💯🎉

williamernest and others added 9 commits August 7, 2018 16:04

chore(text-field): Add screenshot tests

433d18d

chore: update travis to run tests on feat/menu-2 branch (#2852)

c30dae5

feat(menu-surface): Add new component (#2994)

a967f70

feat(menu-surface): Change anchor element (#3067)

a2b06c1

Adds the ability to change the anchor element of a menu surface dynamically. Also adds a function to allow the user to hoist the menu to the body, and calculates the position of the menu in both absolute and fixed positioning.

feat(menu-surface): Emit an event after the menu is opened (#3197)

40d09a8

feat(menu-surface): Add ability to position a menu-surface by x/y (#3261

b44ca8f

)

Revert "chore(text-field): Add screenshot tests"

067d3f9

This reverts commit 01f6b33.

feat(menu): Add new menu (#3221)

d55bdc5

Update .travis.yml

01e071a

kfranqueiro mentioned this pull request Aug 8, 2018

Update Menu page for new Menu in 0.39 material-components/material-components-web-catalog#181

Closed

kfranqueiro suggested changes Aug 8, 2018

View reviewed changes

feat(menu-surface): Update for comments

3f45dd1

feat(menu): Add body position:relative back to demo

522a8a8

feat(menu): Remove outdated comment

6d6ccd9

kfranqueiro reviewed Aug 9, 2018

View reviewed changes

kfranqueiro suggested changes Aug 9, 2018

View reviewed changes

feat(menu): Update for comments

cce8540

kfranqueiro suggested changes Aug 9, 2018

View reviewed changes

kfranqueiro reviewed Aug 9, 2018

View reviewed changes

williamernest and others added 2 commits August 10, 2018 10:28

feat(menu): Update for comments. Add tests.

6abffd4

Merge branch 'master' into feat/menu-2

99fa8f9

Update mdc-menu-surface.test.js

08a9ccc

kfranqueiro suggested changes Aug 10, 2018

View reviewed changes

williamernest and others added 3 commits August 13, 2018 12:15

feat(menu): Update for comments

5202b7c

feat(menu): Add test

c9d198f

Merge branch 'master' into feat/menu-2

87260ed

kfranqueiro approved these changes Aug 13, 2018

View reviewed changes

williamernest and others added 2 commits August 13, 2018 15:17

Merge branch 'master' into feat/menu-2

70f7525

feat(menu): Update dependencies

92ec26b

williamernest merged commit 6439c5b into master Aug 13, 2018

williamernest deleted the feat/menu-2 branch August 13, 2018 22:49

jamesmfriedman mentioned this pull request Aug 23, 2018

MDC 38/39 rmwc/rmwc#281

Closed

48 tasks

+              </div>
+              ```
+              ```js

feat(menu): Adds new menu, menu-surface. #3311

feat(menu): Adds new menu, menu-surface. #3311

Conversation

williamernest commented Aug 7, 2018 • edited by kfranqueiro Loading

mdc-web-bot commented Aug 8, 2018

codecov-io commented Aug 8, 2018 • edited Loading

Codecov Report

mdc-web-bot commented Aug 8, 2018

kfranqueiro 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

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

mdc-web-bot commented Aug 8, 2018

mdc-web-bot commented Aug 9, 2018

mdc-web-bot commented Aug 9, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

mdc-web-bot commented Aug 10, 2018

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

mdc-web-bot commented Aug 10, 2018

mdc-web-bot commented Aug 13, 2018

mdc-web-bot commented Aug 13, 2018

mdc-web-bot commented Aug 13, 2018

mdc-web-bot commented Aug 13, 2018

williamernest commented Aug 7, 2018 •

edited by kfranqueiro

Loading

codecov-io commented Aug 8, 2018 •

edited

Loading

kfranqueiro Aug 9, 2018 •

edited

Loading