Skip to content
Permalink
Browse files

Menubar review

  • Loading branch information
jzaefferer committed May 4, 2011
1 parent 66b96cb commit f519bc0ebb3426cfaa0c918429781586abd87872
Showing with 19 additions and 9 deletions.
  1. +19 −9 tests/visual/menu/menubar.js
@@ -1,11 +1,11 @@
/*
* jQuery UI menubar
*
* backported from Michael Lang's fork: http://www.nexul.com/prototypes/toolbar/demo.html
*
* TODO move to jquery.ui.menubar.js
*/
(function($) {

// TODO take non-menubar buttons into account
// TODO code formatting
$.widget("ui.menubar", {
options: {
buttons: false,
@@ -23,12 +23,14 @@ $.widget("ui.menubar", {
this.element.addClass('ui-menubar ui-widget-header ui-helper-clearfix').attr("role", "menubar");
this._focusable(items);
this._hoverable(items);
// TODO elm is used just once, so the each probably isn't nedded anymore
items.next("ul").each(function(i, elm) {
$(elm).menu({
select: function(event, ui) {
ui.item.parents("ul.ui-menu:last").hide();
self._trigger( "select", event, ui );
self._close();
// TODO what is this targetting? there's probably a better way to access it
$(event.target).prev().focus();
}
}).hide()
@@ -60,6 +62,8 @@ $.widget("ui.menubar", {
return;
}
event.preventDefault();
// TODO can we simplify or extractthis check? especially the last two expressions
// there's a similar active[0] == menu[0] check in _open
if (event.type == "click" && menu.is(":visible") && self.active && self.active[0] == menu[0]) {
self._close();
return;
@@ -90,19 +94,22 @@ $.widget("ui.menubar", {
.attr("role", "menuitem")
.attr("aria-haspopup", "true")
.wrapInner("<span class='ui-button-text'></span>");


// TODO review if these options are a good choice, maybe they can be merged
if (o.menuIcon) {
input.addClass("ui-state-default").append("<span class='ui-button-icon-secondary ui-icon ui-icon-triangle-1-s'></span>");
input.removeClass("ui-button-text-only").addClass("ui-button-text-icon-secondary");
}

if (!o.buttons) {
// TODO ui-menubar-link is added above, not needed here?
input.addClass('ui-menubar-link').removeClass('ui-state-default');
};

});
self._bind({
keydown: function(event) {
// TODO merge the two ifs
if (event.keyCode == $.ui.keyCode.ESCAPE) {
if (self.active && self.active.menu("left", event) !== true) {
var active = self.active;
@@ -117,6 +124,7 @@ $.widget("ui.menubar", {
self._close( event );
}, 100);
},
// TODO change order, focusin first
focusin :function( event ) {
clearTimeout(self.closeTimer);
}
@@ -150,8 +158,7 @@ $.widget("ui.menubar", {
.removeAttr("aria-hidden", "true")
.removeAttr("aria-expanded", "false")
.removeAttr("tabindex")
.unbind("keydown", "blur")
;
.unbind("keydown", "blur");
},

_close: function() {
@@ -182,12 +189,13 @@ $.widget("ui.menubar", {
})
.removeAttr("aria-hidden").attr("aria-expanded", "true")
.menu("focus", event, menu.children("li").first())
// TODO need a comment here why both events are triggered
.focus()
.focusin()
;
.focusin();
this.open = true;
},

// TODO refactor this and the next three methods
_prev: function( event, button ) {
button.attr("tabIndex", -1);
var prev = button.parent().prevAll("li").children( ".ui-button" ).eq( 0 );
@@ -209,7 +217,8 @@ $.widget("ui.menubar", {
firstItem.removeAttr("tabIndex")[0].focus();
}
},


// TODO rename to parent

This comment has been minimized.

Copy link
@micha149

micha149 May 5, 2011

Think this is more like "prevParent" ...

_left: function(event) {
var prev = this.active.parent().prevAll("li:eq(0)").children( ".ui-menu" ).eq( 0 );
if (prev.length) {
@@ -220,6 +229,7 @@ $.widget("ui.menubar", {
}
},

// TODO rename to child (or something like that)

This comment has been minimized.

Copy link
@micha149

micha149 May 5, 2011

... and "nextParent"

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer May 5, 2011

Author Member

How does "go down a level to a child/sub menu" translate to nextParent? I'm not happy with parent/child, but it at least reflects the familar vocabulary of jQuery methods for more or less the same purpose.

This comment has been minimized.

Copy link
@micha149

micha149 May 6, 2011

I thought you suggested "child" as a new name for "_right". The left and right methods move the active element in the first level, or not? Oh, I tested it again right now. The methods also opens a child, of one exists and go to parent, if your are deep enough. In other cases they switch to the next/previous parent. "left" and "right" seems to be nice names. :-D

_right: function(event) {
var next = this.active.parent().nextAll("li:eq(0)").children( ".ui-menu" ).eq( 0 );
if (next.length) {

0 comments on commit f519bc0

Please sign in to comment.
You can’t perform that action at this time.