Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MenuItem, Menu, SubMenu, and MenuBar widgets #104

Closed
wants to merge 36 commits into from
Closed
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
cb2941a
Add Menu widget
Mar 20, 2017
30225d1
Add keypress and focusin events to the menu widget
Mar 20, 2017
78d111a
Fix animation when menu starts opened.
Mar 20, 2017
f2f55aa
Update menu widgets tests
Mar 21, 2017
4c359b0
Add menu comments and `animate` function property
Mar 21, 2017
dd359ee
Menu updates
Mar 21, 2017
2938530
Menu and MenuItem updates
Mar 22, 2017
97135c7
Additional Menu/MenuItem updates
Mar 22, 2017
487f07c
Move menu.css to menu.m.css
Mar 24, 2017
dd1bd20
Move menu colors into common variables
Mar 28, 2017
9745208
Menu and MenuItem updates
Mar 28, 2017
00a2c82
Menu and MenuItem updates
Mar 29, 2017
21d8899
Fix: animate only when `hidden` changes.
Apr 3, 2017
56b962f
Menu/MenuItem updates
Apr 4, 2017
3f8cb65
Add `aria-checked` property to checkbox/radio menu items
Apr 4, 2017
c149e2f
Add Menu widget README.
Apr 4, 2017
7317abf
Menu/MenuItem updates
Apr 4, 2017
06b0706
Key management update.
Apr 5, 2017
90dd3d5
Simplify `_getKeys` method.
Apr 5, 2017
0ec6398
Separate menu and hideable menu.
Apr 5, 2017
7947e12
Implement dropdown functionality.
Apr 6, 2017
5676050
wip menubar widget
Apr 6, 2017
84ee217
Menu fixes
Apr 7, 2017
d2f3a64
Add menu bar unit tests.
Apr 8, 2017
1f75bba
Ignore transpiled `super` calls in constructors.
Apr 8, 2017
0fc3d80
Split `Menu` and `SubMenu` styles.
Apr 8, 2017
6b13dd8
Fix menu item focus logic.
Apr 9, 2017
ce92ceb
MenuItem focus logic update.
Apr 9, 2017
cf2621b
Menu fixes.
Apr 9, 2017
567ecb2
Use `@dojo/core/lang.createHandle` in menu widgets.
Apr 10, 2017
afb9019
Allow menu/trigger CSS in SubMenu to be overridden.
Apr 10, 2017
203b884
Simplify menu `fade` CSS.
Apr 10, 2017
a3ce238
wip menu updates
Apr 13, 2017
ab0c84d
Remove `menuId` property from `MenuItem`.
Apr 13, 2017
dbf3e5c
Menu updates
Apr 19, 2017
b66fb22
Menu updates
Apr 20, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/common/styles/variables.css
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,7 @@
--button-icon-font: 'iconFont';
--button-icon-size: 1.4em;
--button-padding: 0.75em 1.25em;
--menu-bg: var(--dojo-white);
--menu-selected-bg: var(--dojo-blue);
--menu-selected-color: var(--dojo-white);
}
3 changes: 3 additions & 0 deletions src/common/styles/widgets.css
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
@import '../../checkbox/styles/checkbox.m.css';
@import '../../combobox/styles/comboBox.m.css';
@import '../../dialog/styles/dialog.m.css';
@import '../../menu/styles/menu.m.css';
@import '../../menu/styles/menuBar.m.css';
@import '../../menu/styles/subMenu.m.css';
@import '../../radio/styles/radio.m.css';
@import '../../select/styles/select.m.css';
@import '../../slidepane/styles/slidePane.m.css';
Expand Down
3 changes: 2 additions & 1 deletion src/common/tests/intern.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ export const loaderOptions = {
{ name: '@dojo', location: 'node_modules/@dojo' },
{ name: 'grunt-dojo2', location: 'node_modules/grunt-dojo2'},
{ name: 'pepjs', location: 'node_modules/pepjs/dist', main: 'pep' },
{ name: 'maquette', location: 'node_modules/maquette/dist', main: 'maquette' }
{ name: 'maquette', location: 'node_modules/maquette/dist', main: 'maquette' },
{ name: 'sinon', location: 'node_modules/sinon/pkg', main: 'sinon' }
]
};

Expand Down
4 changes: 4 additions & 0 deletions src/common/tests/unit/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,7 @@ import '../../../radio/tests/unit/Radio';
import '../../../slider/tests/unit/Slider';
import '../../../select/tests/unit/Select';
import '../../../select/tests/unit/SelectOption';
import '../../../menu/tests/unit/Menu';
import '../../../menu/tests/unit/MenuBar';
import '../../../menu/tests/unit/MenuItem';
import '../../../menu/tests/unit/SubMenu';
4 changes: 4 additions & 0 deletions src/common/tests/unit/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,9 @@ registerSuite({
assert.isDefined(widgets.Slider);
assert.isDefined(widgets.ComboBox);
assert.isDefined(widgets.Select);
assert.isDefined(widgets.Menu);
assert.isDefined(widgets.MenuBar);
assert.isDefined(widgets.MenuItem);
assert.isDefined(widgets.SubMenu);
}
});
33 changes: 31 additions & 2 deletions src/common/tests/unit/util.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
import * as registerSuite from 'intern!object';
import * as assert from 'intern/chai!assert';
import { Keys } from '../../util';
import * as sinon from 'sinon';
import { Keys, observeViewport } from '../../util';

registerSuite({
name: 'util',

keys() {
beforeEach() {
sinon.spy(window, 'addEventListener');
sinon.spy(window, 'removeEventListener');
},

afterEach() {
(<any> window).addEventListener.restore();
(<any> window).removeEventListener.restore();
},

Keys() {
assert.strictEqual(Keys.Down, 40);
assert.strictEqual(Keys.End, 35);
assert.strictEqual(Keys.Enter, 13);
Expand All @@ -18,5 +29,23 @@ registerSuite({
assert.strictEqual(Keys.Space, 32);
assert.strictEqual(Keys.Tab, 9);
assert.strictEqual(Keys.Up, 38);
},

observeViewport() {
let next = sinon.spy();
const subscriber = observeViewport({ next });
const listener: () => void = (<any> window.addEventListener).args[0][1];
listener();

assert.isTrue((<any> window.addEventListener).calledWith('resize'));
assert.isNumber(next.args[0][0], '`next should be passed the current viewport width.`');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use next.firstCall.calledWith where possible


subscriber.unsubscribe();
assert.isTrue((<any> window.removeEventListener).calledWith('resize'));

next = sinon.spy();
observeViewport({ next }).unsubscribe();
listener();
assert.isFalse(next.called);
}
});
22 changes: 22 additions & 0 deletions src/common/util.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import Observable, { Observer, Subscription } from '@dojo/shim/Observable';

export const enum Keys {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be removed since @smhigley submitted #114?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I just saw that PR had landed.

Down = 40,
End = 35,
Expand All @@ -12,3 +14,23 @@ export const enum Keys {
Tab = 9,
Up = 38
}

export const observeViewport = (function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

let viewportSource: Observable<number>;

return function (observer: Observer<number>): Subscription {
if (!viewportSource) {
viewportSource = new Observable((observer) => {
const listener = function () {
observer.next(document.body.offsetWidth);
};
window.addEventListener('resize', listener);
return function () {
window.removeEventListener('resize', listener);
};
});
}

return viewportSource.subscribe(observer);
};
})();
14 changes: 11 additions & 3 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,33 @@ import Checkbox from './checkbox/Checkbox';
import ComboBox from './combobox/ComboBox';
import Dialog from './dialog/Dialog';
import Label from './label/Label';
import Menu from './menu/Menu';
import MenuBar from './menu/MenuBar';
import MenuItem from './menu/MenuItem';
import Radio from './radio/Radio';
import Select from './select/Select';
import SlidePane from './slidepane/SlidePane';
import Slider from './slider/Slider';
import SubMenu from './menu/SubMenu';
import TabPane from './tabpane/TabPane';
import Textarea from './textarea/Textarea';
import TextInput from './textinput/TextInput';
import Textarea from './textarea/Textarea';

export {
Button,
Checkbox,
ComboBox,
Dialog,
Label,
Menu,
MenuBar,
MenuItem,
Radio,
Select,
SlidePane,
Slider,
SubMenu,
TabPane,
Textarea,
TextInput
TextInput,
Textarea
};
186 changes: 186 additions & 0 deletions src/menu/Menu.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
import { createHandle } from '@dojo/core/lang';
import uuid from '@dojo/core/uuid';
import { v } from '@dojo/widget-core/d';
import { DNode } from '@dojo/widget-core/interfaces';
import ThemeableMixin, { theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable';
import WidgetBase from '@dojo/widget-core/WidgetBase';
import * as css from './styles/menu.m.css';
import { Keys } from '../common/util';

export type Orientation = 'horizontal' | 'vertical';
Copy link
Member

@tomdye tomdye Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would work well as an ENUM as it then avoids the need to cast as Orientation.


export type RoleType = 'menu' | 'menubar';

/**
* @type MenuProperties
*
* Properties that can be set on a Menu component.
*
* @property active Determines whether the menu has focus.
* @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 orientation Determines whether the menu is rendered horizontally.
* @property role The value to use for the menu's `role` property. Defaults to 'menu'.
*/
export interface MenuProperties extends ThemeableProperties {
active?: boolean;
activeIndex?: number;
disabled?: boolean;
id?: string;
orientation?: Orientation;
role?: RoleType;
}

export const enum Operation {
decrease,
increase
}

export const MenuBase = ThemeableMixin(WidgetBase);

@theme(css)
export class Menu extends MenuBase<MenuProperties> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private _active = false;
private _activeIndex = 0;
private _domNode: HTMLElement | null;
private _id = uuid();

constructor() {
/* istanbul ignore next: disregard transpiled `super`'s "else" block */
super();
// TODO: Remove once focus management is implemented.
this.own(createHandle(() => {
this._domNode = null;
}));
}

render(): DNode {
const {
id = this._id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems off to me that this widget will create and store this._id when it may never be used and has no bearing to the rendered ui if an id is specified.

role = 'menu'
} = this.properties;

return v('div', {
classes: this.classes.apply(this, this._getMenuClasses()),
id,
key: 'root',
onfocusin: this._onMenuFocus,
onfocusout: this._onMenuFocusOut,
onkeydown: this._onMenuKeyDown,
role
}, this._renderChildren());
}

protected onElementCreated(element: HTMLElement, key: string) {
if (key === 'root') {
this._domNode = element;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to store a reference to the rendered element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so the widget has a way of knowing whether the current document.activeElement is a descendant of the widget node. When focus management is added, this will go away.

}
}

private _getDefaultOrientation(): Orientation {
const { role = 'menu' } = this.properties;
return role === 'menubar' ? 'horizontal' : 'vertical';
}

private _getKeys() {
const { orientation = this._getDefaultOrientation() } = this.properties;
const isHorizontal = orientation === 'horizontal';

return {
decrease: isHorizontal ? Keys.Left : Keys.Up,
increase: isHorizontal ? Keys.Right : Keys.Down,
tab: Keys.Tab
};
}

private _getMenuClasses() {
const { orientation = this._getDefaultOrientation() } = this.properties;
const classes = [ css.root ];

if (orientation === 'horizontal') {
classes.push(css.horizontal);
}

return classes;
}

private _isActive() {
return this._active || this.properties.active;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the active state not always come from passed in properties?

}

private _moveActiveIndex(operation: Operation) {
const max = this.children.length;
const previousIndex = this._activeIndex;
this._activeIndex = operation === Operation.decrease ?
previousIndex - 1 < 0 ? max - 1 : previousIndex - 1 :
Math.min(previousIndex + 1, max) % max;

this.invalidate();
}

private _onMenuFocus() {
if (!this._isActive()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a controlled pattern I would have expected to see a onRequestFocus function called here such that the parent component can deal with focus / active state.

this._active = true;
this.invalidate();
}
}

private _onMenuFocusOut() {
if (this._isActive()) {
requestAnimationFrame(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to requestanimationframe here? widget renders always occur on the subsequent animation frame

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When clicking within a menu, document.activeElement is equivalent to document.body until the next frame, making the node.contains(document.activeElement) check useless without the rAF.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will direct RAF usage go away if we implement some kind of focus trapping, in the same way that references to the rendered element will disappear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the focus trap jumps through any hoops it needs to track changes to document.activeElement, then widgets shouldn't need to use rAF themselves to determine whether they still have focus.

const node = this._domNode as HTMLElement;
if (node && !node.contains(document.activeElement)) {
this._active = false;
this.invalidate();
}
});
}
}

private _onMenuKeyDown(event: KeyboardEvent) {
const keys = this._getKeys();

switch (event.keyCode) {
case keys.tab:
event.stopPropagation();
this._active = false;
this.invalidate();
break;
case keys.decrease:
event.preventDefault();
event.stopPropagation();
this._moveActiveIndex(Operation.decrease);
break;
case keys.increase:
event.preventDefault();
event.stopPropagation();
this._moveActiveIndex(Operation.increase);
break;
}
}

private _onMenuItemMouseDown(index?: number) {
if (typeof index === 'number') {
this._activeIndex = index;
}
}

private _renderChildren() {
const { activeIndex = this._activeIndex } = this.properties;
const focusIndex = this._isActive() ? activeIndex : undefined;
this._activeIndex = activeIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense.


this.children.filter((child: any) => child && child.properties)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are children rendered inside a <nav> element at any point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, parents will need to render to a <nav> if needed since not every menu warrants a <nav> (e.g., dropdown menus within a parent <nav>).

.forEach((child: any, i) => {
child.properties.active = i === focusIndex;
child.properties.focusable = i === activeIndex;
child.properties.index = i;
child.properties.onMouseDown = this._onMenuItemMouseDown;
});

return this.children;
}
}

export default Menu;
Loading