Menu widget #79

mwistrand · 2017-03-21T03:07:08Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Provides the base Menu widget for #52. Once dojo/widget-core#415 lands, the DropDown widget that extends Menu, and the NavBar widget that relies on both can also be included to completely fulfill #52.

codecov · 2017-03-21T03:12:42Z

Codecov Report

Merging #79 into master will decrease coverage by 0.1%.
The diff coverage is 99.61%.

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
- Coverage     100%   99.89%   -0.11%     
==========================================
  Files          16       18       +2     
  Lines         726      987     +261     
  Branches      202      297      +95     
==========================================
+ Hits          726      986     +260     
- Partials        0        1       +1

Impacted Files	Coverage Δ
src/menu/MenuItem.ts	`100% <100%> (ø)`
src/menu/Menu.ts	`99.53% <99.53%> (ø)`

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 edf2b58...1d2f2e7. Read the comment docs.

agubler · 2017-03-21T09:42:22Z

src/menu/Menu.ts

+
+@theme(css)
+export class Menu extends ThemeableMixin(WidgetBase)<MenuProperties> {
+	protected _wasOpen = false;


I don't think protected properties should be prefixed with _, also is there any reason why this is protected?

Ah; didn't realize about the prefix. There is a lot of shared functionality between Menu and a future DropDown widget that will extend it, in which case using private is not really an option.

agubler · 2017-03-21T09:44:40Z

src/menu/Menu.ts

+}
+
+@theme(css)
+export class Menu extends ThemeableMixin(WidgetBase)<MenuProperties> {


You need to declare a const with the result of ThemableMixin(WidgetBase). Usually with the naming convention const MenuBase = Themeable(WidgetBase) and used to extend like class Menu extends MenuBase<MenuProperties> {.

You also need to export that const. So export const MenuBase = Themeable(WidgetBase).

Yup sorry, I should have put that (basically need to export all the things that are used publicly)

agubler · 2017-03-21T09:45:15Z

src/menu/Menu.ts

+	private _hideTimer: Handle;
+	private _initialRender = true;
+
+	onLabelClick() {


Most of the class functions should be private

These types of functions aren't private in the other widgets?

They should be if they are not an API intended to be overridden to modify behaviour in widgets that extend it. I think @bitpshr did get round to updating Combo.

The visibility throughout was updated as part of the ComboBox PR. Most things are private as @agubler said.

Ok. I'll have to find some way to test those methods that doesn't require using <any> hacks that will break once we start emitting directly to ES6. I should be able to access the event listeners directly of vnode.properties, correct?

All of our existing tests use the hacky (<any> menu)._onFoo() method for testing. I'd just use that for now; we'll have to do a full sweep of the components once @kitsonk's harness lands anyway.

agubler · 2017-03-21T09:46:45Z

src/menu/Menu.ts

+		this._animate(element, hidden);
+	}
+
+	protected _animate(element: HTMLElement, hidden: boolean) {


Is this not possible with css?

Shouldn't the hidden value be taken from this.properties?

Animating to and from auto is not possible with CSS.

The animation is being done using CSS. It just needs an explicit value to animate to besides auto. @mwistrand see comment above re: marginTop and offsetHeight.

Okay. Do you think that we should expose a property that can provide an overriding function for the animation?

@agubler We already do. properties.animate can be a boolean or a function. If it's a boolean, then it enables/disables the default animation. If it's a function, then the built-in _animate function is bypassed in favor of the property function.

I guess my question was, do we think it is valid to accept an overriding function for the animate?

(Sorry if that wasn't clear, I'm happy with the boolean switch)

Gotcha. animate as a function was a last minute add. Given that as long as the height is set in the build-in function, most animations can be handled correctly in CSS, I'll remove this option.

agubler · 2017-03-21T09:51:27Z

src/menu/Menu.ts

+			id,
+			role,
+			classes: this.classes.apply(this, this._getMenuClasses(hidden, Boolean(label))),
+			afterCreate: this._afterCreate,


Can use the onElementCreated and onElementUpdated functions provided by WidgetBase now (dojo/widget-core#382)

Great. I'll update those.

agubler · 2017-03-21T09:59:56Z

src/menu/Menu.ts

+		} = this.properties;
+
+		if (typeof isHidden === 'undefined') {
+			const { hidden = this._getDefaultHidden() } = this.properties;


This can be done in the original destructuring.

Also what is the difference between isHidden and this.properties.hidden?

isHidden is a way to force which state is requested. I'll update the parameter name to make that obvious.

What do you think about using argument defaults?

class Test { properties: any = {}; private _getDefaultHidden(): boolean { return true; } private _toggleDisplay(isHidden = this.properties.hidden || this._getDefaultHidden()) { } }

Could be cleaner if this_getDefaultHidden returns the property hidden if it existed

agubler · 2017-03-21T10:05:48Z

src/menu/MenuItem.ts

+		}), label ? [ label ] : undefined);
+
+		if (this.children.length) {
+			const children = this.children as HNode[];


Why do you need the cast?

I might not anymore. I'll double check, but I did need it at one point.

agubler · 2017-03-21T10:07:13Z

src/menu/MenuItem.ts

+	/**
+	 * Returns an object of aria properties to apply to the widget's DOM element.
+	 */
+	getAriaProperties?: () => { [key: string]: string; };


This doesn't feel like an external property?

It doesn't, but this will be used in a few different ways requiring different ARIA properties. For example, use in Menu requires one set of properties, whereas use with a drop down would require other properties. This seemed like the easiest way to provide any flexibility needed with hard-coding a bunch of scenarios directly within MenuItem.

Do @bitpshr or @smhigley have any suggestions for the ARIA properties of Menu?

@smhigley may have specifics, but you can check https://www.w3.org/WAI/tutorials/menus/. @smhigley and I have been adding a camel-cased property without the "aria" for each ARIA attribute that needs to be set externally. Example. We should stay consistent.

agubler · 2017-03-21T10:07:38Z

src/menu/MenuItem.ts

+}
+
+@theme(css)
+export class MenuItem extends ThemeableMixin(WidgetBase)<MenuItemProperties> {


Same comment about creating a base const for the expression result.

And exporting!

agubler · 2017-03-21T10:08:57Z

src/menu/example/index.html

@@ -0,0 +1,26 @@
+<!DOCTYPE html>


Don't need the specific widget example index.html anymore, this is centralised in common

Thanks, I just remembered I hadn't moved this yet last night.

bitpshr · 2017-03-21T12:25:17Z

src/menu/Menu.ts

+export type Role = 'menu' | 'menubar';
+
+export interface MenuProperties extends ThemeableProperties {
+	/**


These comments should be formatted using TypeDoc.

bitpshr · 2017-03-21T12:41:35Z

src/menu/example/index.html

+<body>
+	<script src="../../../../node_modules/@dojo/loader/loader.min.js"></script>
+	<link rel="stylesheet" href="../../common/styles/widgets.css">
+	<link rel="stylesheet" href="./styles/app.css">


Looks like you put structural styling in the component CSS and theme styling in this example CSS. For now, I'd just put everything in the component CSS. This stuff will be properly separated as part of #51.

Sounds good. I knew that was being separated out eventually, so I just did that here. I'll move it back into the main menu.css, perhaps with notes disambiguating any styles that could go either way.

bitpshr · 2017-03-21T12:42:44Z

src/menu/Menu.ts

+}
+
+@theme(css)
+export class Menu extends ThemeableMixin(WidgetBase)<MenuProperties> {


You also need to export that const. So export const MenuBase = Themeable(WidgetBase).

bitpshr · 2017-03-21T12:44:13Z

src/menu/Menu.ts

+	/**
+	 * Indicates whether the menu is disabled. If true, then the widget will ignore events.
+	 */
+	disabled?: boolean;


Would an entire menu really be disabled? Wouldn't each item within it be disabled instead?

If label is specified, then a MenuItem is created and disabled is passed to it, which keeps it from being expanded.

bitpshr · 2017-03-21T12:50:06Z

src/menu/Menu.ts

+	 * Indicates whether the menu is nested within another menu. Useful for styling, this does not affect
+	 * functionality. Defaults to false.
+	 */
+	nested?: boolean;


Do we need this? If it's just for styling, can we use child selectors to detect if something is nested?

Strictly speaking, we could use child selectors to manage the same thing. But since composes only works with single selectors, I added the extra class to be safe.

bitpshr · 2017-03-21T13:26:38Z

src/menu/MenuItem.ts

+	/**
+	 * Applies only when a URL is provided. If `true`, the URL will be opened in a new window.
+	 */
+	external?: boolean;


Wouldn't a user just put an anchor as the child to this MenuItem? I don't think the MenuItem should handle link behavior.

bitpshr · 2017-03-21T13:27:20Z

src/menu/MenuItem.ts

+	/**
+	 * Returns an object of aria properties to apply to the widget's DOM element.
+	 */
+	getAriaProperties?: () => { [key: string]: string; };


@smhigley may have specifics, but you can check https://www.w3.org/WAI/tutorials/menus/. @smhigley and I have been adding a camel-cased property without the "aria" for each ARIA attribute that needs to be set externally. Example. We should stay consistent.

bitpshr · 2017-03-21T13:31:42Z

src/menu/MenuItem.ts

+}
+
+@theme(css)
+export class MenuItem extends ThemeableMixin(WidgetBase)<MenuItemProperties> {


And exporting!

bitpshr · 2017-03-21T13:32:27Z

src/menu/MenuItem.ts

+			selected ? css.selected : null
+		);
+
+		const labelNode = v('a', assign(ariaProperties, {


You could spread ariaProperties instead of assigning.

I'll update to explicitly list all possible ARIA attributes, which will also make destructuring possible/assigning unnecessary.

bitpshr · 2017-03-21T13:36:55Z

src/menu/MenuItem.ts

+			role: 'menuitem',
+			tabIndex : disabled ? -1 : tabIndex,
+			target: url && external ? '_blank' : undefined
+		}), label ? [ label ] : undefined);


This is odd to me. Menus don't have to contain links so I don't think we should be always creating an anchor. I also don't think the MenuItem should handle hrefs or targets or anything links-specific. It should just put its children in an element so it can still capture events, and that's it. Let the user make the child an anchor if they want.

smhigley

Added some accessibility changes. On a high level, each menu should only have one element in the tab index at any given time. Once focus moves into the menu, arrow keys should be the main mode of navigating through it. So lots of keyboard event handling, yay!

smhigley · 2017-03-29T17:00:54Z

src/menu/MenuItem.ts

+			onkeypress: this.onKeypress,
+			role: 'menuitem',
+			tabIndex : disabled ? -1 : tabIndex
+		}, this.children);


I don't think widget consumers should be able to mess with the tabIndex. It should be fully controlled by the menu widget, and there should only be one focusable menu item per menu (i.e. all but the active menu item should have tabIndex: -1, and the active item should have tabIndex: 0. disabled shouldn't affect the tabindex at all, actually.

smhigley · 2017-03-29T18:46:50Z

src/menu/MenuItem.ts

+		return v('span', {
+			'aria-controls': controls,
+			'aria-expanded': expanded,
+			'aria-hasdropdown': hasDropDown,


This is aria-haspopup, and needs to be converted to a string. I would do 'aria-haspopup': hasDropDown ? 'true' : null, since aria-haspopup="false" is unnecessary, and I think it looks cleaner to leave it off entirely. Also, since we already use hasPopup, it might be better to use that property name for consistency.

Additionally, both aria-expanded and aria-disabled need to be cast to strings

smhigley · 2017-03-29T19:44:15Z

src/menu/Menu.ts

+		} = this.properties;
+
+		const label = this.renderLabel(id);
+		const menu = v('nav', {


This results in multiple nested nav elements if the menu has sub-menus. Additionally, some of the use cases for even non-nested menus probably shouldn't use a nav (e.g. the browser actions menu in the example). I'd be a little inclined to just make it a div and let users wrap it in a v('nav' ...) themselves, but I suppose we could also add a property to control it.

Let's also add an aria-labelledby that points back to the label, just for funsies. (only if it's a dropdown)

smhigley · 2017-03-29T19:47:39Z

src/menu/Menu.ts

+		const {
+			id = uuid(),
+			nested,
+			role = 'menu'


The top-level menu should have a role of menubar, at least if it's visually persistent

Again, this depends on the default use case. If we are assuming that Menu will be used predominantly for main app navigation, then, yes, menubar should be the default.

True. It should probably be tied to whatever determines whether or not we use nav, if we decide to control that.

smhigley · 2017-03-29T20:32:51Z

src/menu/Menu.ts

+			this._hideTimer && this._hideTimer.destroy();
+			this.toggleDisplay(true);
+		}
+	}


When the dropdown can be opened on hover, we should probably consider having it open on focus as well

smhigley · 2017-03-29T20:36:20Z

src/menu/Menu.ts

+		if (!disabled && (key === 'Enter' || key === 13)) {
+			this.toggleDisplay();
+		}
+	}


The menu should show when you hit the down, enter, or space keys. All of them should place focus on the first item in the sub menu when menu opens, and focus should return to the menu trigger before it closes.

Oh, and all the keypress events should probably be keydown so they catch arrow keys

smhigley · 2017-03-29T20:45:59Z

src/menu/MenuItem.ts

+		const { disabled, onKeypress } = this.properties;
+		if (!disabled && typeof onKeypress === 'function') {
+			onKeypress(event);
+		}


I think we should manage key events in a little more depth:

up/down arrow keys should move focus to the previous or next menu item, and wrap beginning <--> end.

left/right should do the same on horizontal menus

space/enter: activates the menu item and closes the dropdown menu (if applicable)

space on a checkbox: toggles the checkbox but doesn't close the menu

escape: closes the menu

smhigley · 2017-03-29T20:47:57Z

src/menu/Menu.ts

+	nested?: boolean;
+	onRequestHide?: () => void;
+	onRequestShow?: () => void;
+	role?: Role;


We should probably also have an orientation attribute that defaults to horizontal, at least for top-level menus.

An orientation attribute makes sense, but given the generic use case for the Menu widget, do we really want to make it the default? @bitpshr?

Yeah, I wasn't all that sure about horizontal either. I don't have strong opinions either way, I just suggested horizontal because I think that's the default browser assumption for menus.

smhigley · 2017-03-29T21:00:07Z

src/menu/Menu.ts

+				overrideClasses: overrideClasses || css,
+				onClick: this.onLabelClick,
+				onKeypress: this.onLabelKeypress
+			}, [ label ]);


This should probably have an id, so the dropdown can refer back to it

tomdye · 2017-04-04T07:21:51Z

src/menu/MenuItem.ts

+			selected ? css.selected : null
+		);
+
+		return v(tag, assign(Object.create(null), properties, {


It is necessary to pass the tag through here, I don't believe we are doing this anywhere else.

Is there any need for the assign call here over simple passing properties ?

I'd be fine with internally determining the tag name based on the specified type property. As for assign, it cleanly separates the properties that are specific to and required by MenuItem from those needed only by the underlying DOM node and have nothing to do with Menu functionality. Also, since other widgets (e.g., Button) use generic property names for the aria- properties, I tried to do the same thing here.

tomdye · 2017-04-04T07:23:04Z

src/menu/MenuItem.ts

+			'aria-haspopup': hasPopup ? 'true' : undefined,
+			'aria-disabled': String(disabled),
+			classes,
+			key: 'menu-item',


I think that the key should be set by the parent or should at least use the menu item id or there will be no difference.

Hmm. Both ComboBox and SlidePane use a static key for controlling how onElementCreated behaves.

tomdye · 2017-04-04T07:23:32Z

src/menu/MenuItem.ts

+		}), this.children);
+	}
+
+	protected onClick(event: MouseEvent) {


these are usually private

Correct; initially I included all the functionality for dropdowns within Menu, but after the widget logic became too complicated decided that it might be better to have a dedicated DropDown widget that extends Menu, in which case using private limits what can be done.

tomdye · 2017-04-04T07:24:36Z

src/menu/MenuItem.ts

+	protected onKeydown(event: KeyboardEvent) {
+		const { disabled, onKeydown } = this.properties;
+		if (!disabled) {
+			if (event.keyCode === 32) { // space


do you think it would be better to define a const SPACE_KEY = 32 and use that here instead of needing a comment?

Thanks for catching that. I thought I'd updated all of those.

tomdye · 2017-04-04T07:26:01Z

src/menu/MenuItem.ts

+			if (event.keyCode === 32) { // space
+				(<HTMLElement> event.target).click();
+			}
+			typeof onKeydown === 'function' && onKeydown(event);


The pattern elsewhere for consistency is onKeyDown && onKeyDown(event).
Can onKeydown take any form other than undefined or a function? Also do you think down should be changed to have a capital D?

I'll update to onKeyDown since that's the pattern used everywhere else.

tomdye · 2017-04-04T07:31:34Z

src/menu/styles/menu.m.css

@@ -0,0 +1,66 @@
+@import '../../common/styles/variables.css';
+
+.container {


can you change this to root please?

tomdye · 2017-04-04T07:32:20Z

src/menu/styles/menu.m.css

+	transition: height 0.5s ease-in-out;
+}
+
+.hidden { height: 0; }


I think that these classes should be using the base.css classes. We should have a consistent approach to show / hidden

Unfortunately .hidden in base.css uses display: none, which makes animation difficult; hence the need for the local classes. One option would be to change the class name to collapsed, but when applied to drop downs hidden feels more natural.

tomdye · 2017-04-04T07:32:41Z

src/menu/styles/menu.m.css

+	box-sizing: border-box;
+	overflow: hidden;
+	padding: 0 15px;
+	transition: height 0.5s ease-in-out;


Can you please use the short animation time variable here

tomdye · 2017-04-04T07:34:18Z

src/menu/MenuItem.ts

+	protected onClick(event: MouseEvent) {
+		const { disabled, onClick } = this.properties;
+		if (!disabled && typeof onClick === 'function') {
+			onClick(event);


Would it be more advantageous to return the item id / key here instead of the click event? There's likely no need to expose the dom click event to the parent Menu.

The Menu widget does not need access to the event, but there may be instances in which an application needs to call event.preventDefault on a MenuItem click event.

- Use `properties.hidden` directly instead of passing it around. - Disambiguate `isHidden` to `requestShow` in `_toggleDisplay`. - Create an export base classes from Menu and MenuItem. - Remove underscore prefix from protected class members. - Fix doc comments for Menu/MenuItem properties. - Fix MenuItem child type.

- Remove the ability to specify `animate` as a function. - Remove `getAriaProperties` in favor of individual properties. - Simplify the MenuItem widget.

- Remove unused import - Remove menu/example.html - Fix example page - Use `KeyboardEvent#keyCode` where `key` is unavailable - Consolidate menu CSS into a single module - Ensure menu is scrolled to top when animating open

- Use `onElementCreated` and `onElementUpdated` - Correct use of `KeyboardEvent#keyCode` - Privacy updates

`MenuItem`: - Allow tag and vnode properties to be set. - Fix ARIA properties. - Allow different `role`s to be applied. `Menu`: - Update keyboard handling to more accurately match the WAI-ARIA spec. - Add handling for right/left arrows, space key. - Internalize active index management. - Add an `orientation` property.

- Rename event listener names for consistency with other widgets. - Change `container`/`nestedMenuContainer` CSS classes to `root`/`nestedMenuRoot`, respectively. - Use CSS variable for menu animation duration. - Simplify function property checks. - Only move focus from the label into the menu when navigating via the keyboard. - Add `hideOnActivate` options. - Always expand submenus when the designated "descend" key is pressed.

smhigley

Seems like enough for round 2 :)

smhigley · 2017-04-04T21:31:07Z

src/menu/Menu.ts

+			onfocusout: this.onMenuFocusOut,
+			onkeydown: this.onMenuKeyDown,
+			role,
+			tabIndex: this.state.active || label ? -1 : 0


I actually don't think the container needs to have a tabIndex at all. Semantically it doesn't make much sense, it creates extra work managing initial focus, and it should be more straightforward to just have a single menu item with tabIndex: 0 in menus without a label.

There are also some weird things happening with my ability to focus on the second menu after I've opened and closed its sub-menu. I think everything would be simplified by always having exactly one top-level menu item with tabIndex: 0, and directly moving focus as little as possible.

Based on our private conversation, I can update MenuItem to store a reference to its DOM node so that focus can be moved out of onElementUpdated and changed in response to keyboard events. That way, this ~~hack~~ workaround can be removed.

smhigley · 2017-04-04T21:51:12Z

src/menu/Menu.ts

+		} = this.properties;
+
+		if (id) {
+			this._id = id;


Could use destructuring instead, i.e.

const { id = this._id, ... } = this.properties

smhigley · 2017-04-04T22:04:58Z

src/menu/MenuItem.ts

+
+	protected onElementUpdated(element: HTMLElement, key: string) {
+		if (key === 'root') {
+			this.properties.active && element.focus();


I think one problem is properties.active is controlling both current focus and tabIndex. There are times when a menu item should have tabIndex: 0 without being the currently focused page element.

This is also messing with normal focus behavior. For example, clicking on something else should move focus, but it doesn't b/c this sets focus back on the menu.

MenuItems now have both an active property for determining whether it should be focused and a focusable property for determining the tabIndex.

smhigley · 2017-04-04T22:06:45Z

src/menu/MenuItem.ts

+			'aria-controls': controls,
+			'aria-expanded': String(expanded),
+			'aria-haspopup': hasPopup ? 'true' : undefined,
+			'aria-disabled': String(disabled),


aria-expanded and aria-disabled should only show up if true: 'aria-expanded': expanded ? 'true' : null

Hmm. The WAI-ARIA example always shows aria-expanded.

@mwistrand yeah, sorry, it should be aria-expanded: hasPopup ? String(expanded) : null

smhigley · 2017-04-04T22:14:05Z

src/menu/MenuItem.ts

+		return v(tag, assign(Object.create(null), properties, itemProperties), this.children);
+	}
+
+	protected onClick(event: MouseEvent) {


Clicking on a menu item also needs to update it to active, same as navigating to it with the keyboard.

smhigley · 2017-04-04T22:24:29Z

src/menu/MenuItem.ts

+		const { disabled, onKeyDown } = this.properties;
+		if (!disabled) {
+			if (event.keyCode === SPACE_KEY) {
+				(<HTMLElement> event.target).click();


Do you need to trigger a fake click on event.target? Why not just do onClick && onClick(event)?

If clicking a MenuItem just opens a link in a new window, for example, then calling onClick 1) requires the MenuItem to manually navigate to the underlying URL within its onClick handler and 2) potentially causes problems with popup blockers that are circumvented by just calling click().

By "popup blocker", I mean built-in browser functionality.

smhigley · 2017-04-04T22:25:50Z

src/menu/MenuItem.ts

+			if (event.keyCode === SPACE_KEY) {
+				(<HTMLElement> event.target).click();
+			}
+			onKeyDown && onKeyDown(event);


This should probably be the first thing called, since onkeydown would be called before onclick with the enter key.

smhigley · 2017-04-04T22:35:33Z

src/menu/Menu.ts

+			return true;
+		}
+
+		return label ? true : false;


I'm not sure why you need the if (label && disabled) block, since it still boils down to label ? true : false.

lol. You're right.

smhigley · 2017-04-04T22:56:59Z

src/menu/Menu.ts

+				this.toggleDisplay();
+			}
+			else if (key === keys.descend) {
+				this.toggleDisplay(true);


Also need to preventDefault if the key is up/down to prevent page scrolling, same for onkeydown on menu items

smhigley · 2017-04-04T23:04:59Z

src/menu/example/index.ts

+						}
+					},
+					onClick: toggleSelected,
+					selected: <boolean> this.state[`${key}Selected`]


These seem like they're functioning as checkboxes with the selected property. Should they have role: checkbox even though the text seems to imply otherwise?

That's a good point. Since they are using selected, they should have role: checkbox.

- general cleanup - remove tabIndex from menu container - reset active index on hide - move focus on click

- Prevent the default action when the down arrow key is pressed while the menu trigger is focused. - Add `parentOrientation` property to ensure that vertical submenus of horizontal menus are opened with down arrow key instead of the right arrow key.

mwistrand · 2017-04-10T13:36:40Z

Closing in favor of #104.

agubler requested changes Mar 21, 2017

View reviewed changes

bitpshr suggested changes Mar 21, 2017

View reviewed changes

dylans added this to the 2017.03 milestone Mar 22, 2017

mwistrand force-pushed the 52-menu-widget branch 3 times, most recently from f7a8679 to 52f22b9 Compare March 28, 2017 20:35

smhigley suggested changes Mar 29, 2017

View reviewed changes

dylans modified the milestones: 2017.03, 2017.04 Apr 2, 2017

mwistrand force-pushed the 52-menu-widget branch from 393cdbe to 5294a25 Compare April 3, 2017 20:55

tomdye requested changes Apr 4, 2017

View reviewed changes

Matt Wistrand added 15 commits April 4, 2017 14:34

Add Menu widget

5c69181

Add keypress and focusin events to the menu widget

de3ab26

Fix animation when menu starts opened.

aaa533d

Update menu widgets tests

38500bc

Add menu comments and animate function property

268d7df

Menu and MenuItem updates

48398b0

- Remove the ability to specify `animate` as a function. - Remove `getAriaProperties` in favor of individual properties. - Simplify the MenuItem widget.

Additional Menu/MenuItem updates

a0292e5

- Remove unused import - Remove menu/example.html - Fix example page - Use `KeyboardEvent#keyCode` where `key` is unavailable - Consolidate menu CSS into a single module - Ensure menu is scrolled to top when animating open

Move menu.css to menu.m.css

035c8e6

Move menu colors into common variables

8223d6b

Menu and MenuItem updates

9e2e277

- Use `onElementCreated` and `onElementUpdated` - Correct use of `KeyboardEvent#keyCode` - Privacy updates

Fix: animate only when hidden changes.

3911c93

Add aria-checked property to checkbox/radio menu items

d291953

mwistrand force-pushed the 52-menu-widget branch from 6d5884c to d291953 Compare April 4, 2017 19:35

Add Menu widget README.

e5874fd

smhigley suggested changes Apr 4, 2017

View reviewed changes

Matt Wistrand added 3 commits April 5, 2017 13:14

Menu/MenuItem updates

68f287c

- general cleanup - remove tabIndex from menu container - reset active index on hide - move focus on click

Key management update.

3e113fe

- Prevent the default action when the down arrow key is pressed while the menu trigger is focused. - Add `parentOrientation` property to ensure that vertical submenus of horizontal menus are opened with down arrow key instead of the right arrow key.

Simplify _getKeys method.

1d2f2e7

mwistrand mentioned this pull request Apr 10, 2017

MenuItem, Menu, SubMenu, and MenuBar widgets #104

Closed

3 tasks

mwistrand closed this Apr 10, 2017

tomdye mentioned this pull request Jan 30, 2020

Create Material Theme #1048

Closed

29 tasks

		@@ -0,0 +1,66 @@
		@import '../../common/styles/variables.css';

		.container {

Menu widget #79

Menu widget #79

Conversation

mwistrand commented Mar 21, 2017

codecov bot commented Mar 21, 2017 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

agubler Mar 21, 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

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

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

smhigley 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

smhigley Mar 29, 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

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

codecov bot commented Mar 21, 2017 •

edited

Loading

agubler Mar 21, 2017 •

edited

Loading

smhigley Mar 29, 2017 •

edited

Loading