`MenuItem`, `Menu`, `SubMenu`, and `MenuBar` widgets #104

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

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:

Continues the work in #79, simplifying the Menu widget by splitting it into two widgets: Menu for managing just the list of menu items and their interactions, and SubMenu (admittedly not the best name; I'm very open to suggestions) for rendering a trigger that toggles a menu, whether that be an inlined menu, a drop down, or a popup. Also includes a simple MenuBar widget that takes a numerical breakpoint, beneath which all children are rendered to a SlidePane.

Resolves #52

codecov · 2017-04-10T13:47:35Z

Codecov Report

Merging #104 into master will decrease coverage by 0.04%.
The diff coverage is 99.25%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   99.78%   99.73%   -0.05%     
==========================================
  Files          18       20       +2     
  Lines         920     1131     +211     
  Branches      274      335      +61     
==========================================
+ Hits          918     1128     +210     
- Partials        2        3       +1

Impacted Files	Coverage Δ
src/menu/MenuItem.ts	`100% <100%> (ø)`
src/menu/MenuBar.ts	`97.36% <97.36%> (ø)`
src/menu/Menu.ts	`98.95% <98.95%> (ø)`
src/menu/SubMenu.ts	`99.5% <99.5%> (ø)`
src/slider/Slider.ts	`100% <0%> (ø)`	⬆️
src/label/Label.ts	`100% <0%> (ø)`	⬆️
src/textinput/TextInput.ts	`100% <0%> (ø)`	⬆️
src/button/Button.ts	`100% <0%> (ø)`	⬆️
src/main.ts	`100% <0%> (ø)`	⬆️
src/textarea/Textarea.ts	`100% <0%> (ø)`	⬆️
... and 11 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 3ae7f14...b66fb22. Read the comment docs.

bitpshr

Looking really good. Some questions and comments.

bitpshr · 2017-04-13T02:15:59Z

src/menu/Menu.ts

+ *
+ * Properties that can be set on a Menu component.
+ *
+ * @property active				Determines whether the menu has focus.


We've been using spaces for TypeDoc comments.

bitpshr · 2017-04-13T02:17:19Z

src/menu/Menu.ts

+ * @property activeIndex		Determines the index of the focused item.
+ * @property disabled			Determines whether the menu is disabled.
+ * @property id					The widget ID. Defaults to a random string.
+ * @property nested				Indicates whether the menu is nested within another menu.


Would a nested menu not be a SubMenu? It looks like this is only used for styling.

It is only used for styling, and since SubMenu renders its menu to a container node it can control when animating, and since there are other classes for positioning, this is probably unnecessary. I'll remove it.

bitpshr · 2017-04-13T02:21:45Z

src/menu/Menu.ts

+	private _renderChildren() {
+		const { activeIndex = this._activeIndex, id = this._id } = this.properties;
+		const focusIndex = this._isActive() ? activeIndex : undefined;
+		this._activeIndex = activeIndex;


Let's say a parent component passes in an activeIndex then the active element is changed via the arrow keys. Wouldn't the new index immediately be overwritten since the parent doesn't have any way to know to clear out the activeIndex property it was passing in?

The parent would need a means of determining whether the activeIndex should be set. For example, SubMenu uses activeIndex: this.properties.hidden ? 0 : undefined, so control is handed over to Menu as soon as it is visible. In practice, parents should avoid setting activeIndex as much as possible, and it was added specifically so that SubMenu would have a way of indicating to the child menu that keyboard navigation should start from the first item after closing the menu.

@smhigley and I were discussing "internal use" properties that naturally arise from creating distinct components that are intended to work with each other, and how it would be nice if we had a natural way of marking them as such. One option is to use the format __propertyName__ as is done with the __render__ method, but I can open an issue for discussion.

Ok, makes sense.

bitpshr · 2017-04-13T02:28:53Z

src/menu/Menu.ts

+		const index = parseInt(itemNode.getAttribute('data-dojo-index') || '', 10);
+		const menuId = itemNode.getAttribute('data-dojo-menuid');
+
+		if (menuId === id && !isNaN(index)) {


I wonder if it would be cleaner to put a mouse down handler on each child menu item. That way you can be sure any item triggering this handler is a child of this menu, so these checks could go away.

I added the menuId property to prevent menus from responding to events within nested menus (for example, clicking an item within a dropdown menu should not cause an ancestor MenuBar to update its own index). We could deliberately add a child.properties.onMouseDown to menu items from within _renderChildren, which might solve this problem.

bitpshr · 2017-04-13T02:31:17Z

src/menu/MenuBar.ts

+		return v('div', {
+			classes: this.classes(css.navBar, css.slidePane)
+		}, [
+			v('button', {


Part of me feels like we should accept any arbitrary DNode and use that as the trigger for the SlidePane instead of always using a button.

Since the trigger needs to open/close the slide pane on click, how about a function property that takes the click handler and returns a DNode?
slidePaneTrigger?: string | ((onClick: () => void) => DNode)

bitpshr · 2017-04-13T02:37:31Z

src/menu/MenuBar.ts

+	private _observeViewport() {
+		// TODO: If the current vw is stored as a private variable, then it would
+		// be possible to invalidate only when the vw crosses the breakpoint.
+		const viewportSubscription = viewportSource.subscribe({


I agree with your TODO here, it would be good to conditionally invalidate.

bitpshr · 2017-04-13T02:40:52Z

src/menu/MenuItem.ts

+export const MenuItemBase = ThemeableMixin(WidgetBase);
+
+const SPACE_KEY = 32;
+const ENTER_KEY = 13;


We should really pull these out into a common module. I think this, TabPane, ComboBox, and a few others use internal key maps, so they can all be updated as well. You don't have to do that as part of this PR, but I won't stop you :)

Agreed. I'll add a Keys map and the vw observer to a common/util.ts module, and we can expand on that/separate functionality later if the need arises.

bitpshr · 2017-04-13T02:41:52Z

src/menu/MenuItem.ts

+	menuId?: string;
+	onClick?: (event: Event) => void;
+	onKeyDown?: (event: KeyboardEvent) => void;
+	properties?: VirtualDomProperties;


What was the motivation for this properties bag?

Since the user can specify the tag used for the item's DOM node, it makes sense to also allow properties specific to that tag to be used (e.g., href with <a>). It also keeps those properties separate from those required by MenuItem to function properly.

bitpshr · 2017-04-13T02:42:59Z

src/menu/MenuItem.ts

+
+							if (this.properties.active) {
+								if (visibility === 'hidden') {
+									if (callCount > 30) {


A little confused by this. What's the purpose of the callCount check here?

It's just a safety guard against infinitely checking whether a hidden MenuItem has become visible, since the MenuItem does not control its own visibility.

Got it. Maybe pull that out into a constant.

bitpshr · 2017-04-13T02:47:24Z

src/menu/SubMenu.ts

+	label: DNode;
+	labelId?: string;
+	labelStyles?: any;
+	menuStyles?: any;


This is a little confusing. It's not obvious that a SubMenu creates a Menu internally, so a developer could naively pass overrideClasses to the SubMenu thinking they would theme it properly. Maybe you could pass this.properties.overrideClasses to the Menu that's created and lose menuStyles altogether?

Since Menu can be used independently of SubMenu, I separated the styles into two different CSS files. If we need to merge the two, or if there is a way to clarify the current implementation, I'll make the necessary changes.

I vote to merge them so that SubMenu could pass its overrideClasses straight on to Menu, but it's your call.

bitpshr

Looking great. A few more comments.

bitpshr · 2017-04-19T13:47:19Z

src/common/styles/variables.css.d.ts

@@ -0,0 +1 @@
+export default {};


We shouldn't be generating .d.ts files for variables. Looks like this may've been included by mistake.

bitpshr · 2017-04-19T13:50:37Z

src/common/util.ts

@@ -0,0 +1,32 @@
+import Observable, { Observer, Subscription } from '@dojo/shim/Observable';
+
+export const enum Keys {


Should this be removed since @smhigley submitted #114?

Yes. I just saw that PR had landed.

bitpshr · 2017-04-19T13:50:55Z

src/common/util.ts

+	up = 38
+};
+
+export const observeViewport = (function () {


bitpshr · 2017-04-19T14:37:13Z

src/menu/Menu.ts

+
+export type Orientation = 'horizontal' | 'vertical';
+
+export type Role = 'menu' | 'menubar';


This should probably be MenuRole. See #106.

MenuRole or RoleType?

Ah, yeah. I guess RoleType.

bitpshr · 2017-04-19T17:24:51Z

src/menu/MenuItem.ts

+ * @property properties     Additional properties for the widget's vnode.
+ * @property selected       Determines whether the widget is marked as selected.
+ * @property tag            The HTML tag name to use for the widget's vnode. Defaults to 'a'.
+ * @property type           The type of the menu item. Defaults to 'item'.


Should this be role to stay consistent?

It was role originally, but @smhigley and I agreed that allowing users to specify item, checkbox, or radio instead of menuitem, menuitemcheckbox or menuitemradio would be better, in which case role became somewhat misleading.

bitpshr · 2017-04-19T17:25:49Z

src/menu/MenuItem.ts

+					// within the next task, whereas others may take multiple tasks before the element switches
+					// to visible.
+					let callCount = 0;
+					const scheduleFocus = () => {


Why did you have to wrap the rAF call in an anonymous function?

Since rAF may need to be called multiple times before the element becomes visible in certain browsers, depending on the animation.

bitpshr · 2017-04-19T17:35:05Z

src/menu/Menu.ts

+export const MenuBase = ThemeableMixin(WidgetBase);
+
+@theme(css)
+export class Menu extends MenuBase<MenuProperties> {


We should probably have a custom element descriptor for all of these components.

- 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.

- 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.

Simplify the `Menu` widget to do nothing more than render a list of menu children and manage the keyboard/mouse event interactions with them. Create a separate `SubMenu` widget that renders a menu item that toggles the menu in-and-out of view.

Update `SubMenu` with dropdown functionality and the beginnings of popup functionality.

- Add guard to focusout handler to prevent destroyed widgets from being manually inactivated. - Change `animate` to `animation`. - Toggle menu with space key. - `MenuItem`s default to `a` tag. - Implement `hideOnBlur` property. - Respond to submenu key events only when active - Update privacy of MenuItem methods. - Stop propagation when an arrow key is pressed on a SubMenu trigger to prevent navigation within a parent menu. - Focus menu items within a rAF call in order to give any animations time to complete.

kitsonk · 2017-10-06T12:52:41Z

Let either update and land this, or close it.

tomdye · 2017-11-07T17:22:22Z

Closing this for now as it has been stagnant for some time. Can be used as a reference in future when working on a Menu widget.

mwistrand mentioned this pull request Apr 10, 2017

Menu widget #79

Closed

3 tasks

dylans added this to the 2017.04 milestone Apr 10, 2017

bitpshr suggested changes Apr 13, 2017

View reviewed changes

bitpshr suggested changes Apr 19, 2017

View reviewed changes

Matt Wistrand added 25 commits April 19, 2017 12:56

Add Menu widget

cb2941a

Add keypress and focusin events to the menu widget

30225d1

Fix animation when menu starts opened.

78d111a

Update menu widgets tests

f2f55aa

Add menu comments and animate function property

4c359b0

Menu and MenuItem updates

2938530

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

Additional Menu/MenuItem updates

97135c7

- 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

487f07c

Move menu colors into common variables

dd1bd20

Menu and MenuItem updates

9745208

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

Fix: animate only when hidden changes.

21d8899

Add aria-checked property to checkbox/radio menu items

3f8cb65

Add Menu widget README.

c149e2f

Menu/MenuItem updates

7317abf

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

Key management update.

06b0706

- 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.

90dd3d5

Separate menu and hideable menu.

0ec6398

Simplify the `Menu` widget to do nothing more than render a list of menu children and manage the keyboard/mouse event interactions with them. Create a separate `SubMenu` widget that renders a menu item that toggles the menu in-and-out of view.

Implement dropdown functionality.

7947e12

Update `SubMenu` with dropdown functionality and the beginnings of popup functionality.

wip menubar widget

5676050

Add menu bar unit tests.

d2f3a64

Ignore transpiled super calls in constructors.

1f75bba

eheasley added the beta2 label May 30, 2017

eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017

eheasley added beta3 and removed beta2 labels Jun 6, 2017

dylans modified the milestones: 2017.06, 2017.07 Jul 4, 2017

dylans modified the milestones: 2017.07, 2017.08 Aug 4, 2017

kitsonk modified the milestones: 2017.08, 2017.09 Sep 4, 2017

kitsonk assigned bitpshr Oct 6, 2017

kitsonk added beta4 and removed beta3 labels Oct 6, 2017

kitsonk modified the milestones: 2017.09, 2017.10 Oct 6, 2017

kitsonk modified the milestones: 2017.10, beta.4 Oct 30, 2017

tomdye closed this Nov 7, 2017

kitsonk mentioned this pull request Nov 29, 2017

Menu widget #52

Closed

This pull request was closed.

		@@ -0,0 +1,32 @@
		import Observable, { Observer, Subscription } from '@dojo/shim/Observable';

		export const enum Keys {


		export type Orientation = 'horizontal' \| 'vertical';

		export type Role = 'menu' \| 'menubar';

MenuItem, Menu, SubMenu, and MenuBar widgets #104

MenuItem, Menu, SubMenu, and MenuBar widgets #104

Conversation

mwistrand commented Apr 10, 2017

codecov bot commented Apr 10, 2017 • edited Loading

Codecov Report

bitpshr 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

mwistrand Apr 13, 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

bitpshr 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

kitsonk commented Oct 6, 2017

tomdye commented Nov 7, 2017

`MenuItem`, `Menu`, `SubMenu`, and `MenuBar` widgets #104

`MenuItem`, `Menu`, `SubMenu`, and `MenuBar` widgets #104

codecov bot commented Apr 10, 2017 •

edited

Loading

mwistrand Apr 13, 2017 •

edited

Loading