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

Conversation

mwistrand
Copy link
Contributor

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

@mwistrand mwistrand mentioned this pull request Apr 10, 2017
3 tasks
@codecov
Copy link

codecov bot commented Apr 10, 2017

Codecov Report

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

Impacted file tree graph

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

@dylans dylans added this to the 2017.04 milestone Apr 10, 2017
Copy link
Member

@bitpshr bitpshr left a comment

Choose a reason for hiding this comment

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

Looking really good. Some questions and comments.

src/menu/Menu.ts Outdated
*
* Properties that can be set on a Menu component.
*
* @property active Determines whether the menu has focus.
Copy link
Member

Choose a reason for hiding this comment

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

We've been using spaces for TypeDoc comments.

src/menu/Menu.ts Outdated
* @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.
Copy link
Member

Choose a reason for hiding this comment

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

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

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

src/menu/Menu.ts Outdated
private _renderChildren() {
const { activeIndex = this._activeIndex, id = this._id } = 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.

src/menu/Menu.ts Outdated
const index = parseInt(itemNode.getAttribute('data-dojo-index') || '', 10);
const menuId = itemNode.getAttribute('data-dojo-menuid');

if (menuId === id && !isNaN(index)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mwistrand mwistrand Apr 13, 2017

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

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({
Copy link
Member

Choose a reason for hiding this comment

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

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

export const MenuItemBase = ThemeableMixin(WidgetBase);

const SPACE_KEY = 32;
const ENTER_KEY = 13;
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 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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

What was the motivation for this properties bag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if (this.properties.active) {
if (visibility === 'hidden') {
if (callCount > 30) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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 just a safety guard against infinitely checking whether a hidden MenuItem has become visible, since the MenuItem does not control its own visibility.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Maybe pull that out into a constant.

label: DNode;
labelId?: string;
labelStyles?: any;
menuStyles?: any;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@bitpshr bitpshr left a comment

Choose a reason for hiding this comment

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

Looking great. A few more comments.

@@ -0,0 +1 @@
export default {};
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,32 @@
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.

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.

src/menu/Menu.ts Outdated

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

export type Role = 'menu' | 'menubar';
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be MenuRole. See #106.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MenuRole or RoleType?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah. I guess RoleType.

* @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'.
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 role to stay consistent?

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.

Matt Wistrand added 25 commits April 19, 2017 12:56
- 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.
@eheasley eheasley added the beta2 label May 30, 2017
@eheasley eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017
@eheasley eheasley added beta3 and removed beta2 labels Jun 6, 2017
@dylans dylans modified the milestones: 2017.06, 2017.07 Jul 4, 2017
@dylans dylans modified the milestones: 2017.07, 2017.08 Aug 4, 2017
@kitsonk kitsonk modified the milestones: 2017.08, 2017.09 Sep 4, 2017
@kitsonk kitsonk added beta4 and removed beta3 labels Oct 6, 2017
@kitsonk kitsonk modified the milestones: 2017.09, 2017.10 Oct 6, 2017
@kitsonk
Copy link
Member

kitsonk commented Oct 6, 2017

Let either update and land this, or close it.

@kitsonk kitsonk modified the milestones: 2017.10, beta.4 Oct 30, 2017
@tomdye
Copy link
Member

tomdye commented Nov 7, 2017

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.

@tomdye tomdye closed this Nov 7, 2017
@kitsonk kitsonk mentioned this pull request Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants