Skip to content

Menubar #829

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.

Sign up for GitHub

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

Closed
wants to merge 33 commits into from
Closed

Menubar #829

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0f33062
Adds unit test: correct sub-menu-less menu item event behavior
Nov 19, 2012
82d0056
Convert from bind() to _on()
Nov 19, 2012
15bd67f
menubar: remove remaining bind() calls; code standards compliance.
Nov 20, 2012
d4ca89b
menubar: rm stray, empty conditional
Dec 1, 2012
f30c853
menubar: rm namespacing on events in _on
Dec 3, 2012
ec67b65
menubar: decompose _create
Dec 4, 2012
c57319c
Revert "menubar: decompose _create"
Dec 8, 2012
36b5135
menubar: spacing / style fix
Dec 8, 2012
1a9f136
menubar: add visual test
Dec 8, 2012
ab98a00
menubar: restore keyboard navigation
Dec 8, 2012
2fe73a1
menubar: keyboard focus / mouse interaction
Dec 8, 2012
baf9882
menubar: more spaces
Dec 8, 2012
0700982
menubar: visual test: add to index page
Dec 9, 2012
2500c96
menubar: kbd / mouse interaction
Dec 9, 2012
bb87639
menubar: spacing and formatting
Dec 10, 2012
b74695a
menubar: rm erroneously committed .orig file
Dec 10, 2012
706d882
menubar: spacing
Dec 10, 2012
8237fb2
menubar: massive refactor for readability
Dec 30, 2012
1af1f8c
meubar: formatting per JQuery style guide
Jan 17, 2013
908fea5
menubar: mark active menuItem with .ui-state-focus
Jan 18, 2013
bb8e2e0
menubar: re-open submenu when returning to it after hover on menu-les…
Jan 18, 2013
0efe66e
menubar: relocate focus event onto *item* v. menuItem
Jan 18, 2013
fdc200d
menubar: fix pixel-shifting visual error
Jan 18, 2013
aafdc17
menubar: apply drop-down glyph only on menus w/ subMenu
Jan 20, 2013
1e8089b
LEFT cursor in an expanded menu approximates ESCAPE
Mar 14, 2013
09d51e8
Remove unused variable: seenFirstItem
Mar 16, 2013
a4a95cc
Do not remove tabIndex varaible
Mar 16, 2013
b2a3814
rm stray debugger
Mar 16, 2013
bab98f2
Change selector for next .ui-menubar-link -> .ui-button
Mar 16, 2013
79b06b2
Repair LEFT cursor
Mar 22, 2013
480de7f
Refactor _move by menuItems knowing their neighbors
Mar 22, 2013
56f6469
Method rename
Mar 22, 2013
100f552
Correct submenus triggering bad focusout behavior
Mar 22, 2013
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
21 changes: 21 additions & 0 deletions tests/unit/menubar/menubar_events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,25 @@ test( "handle click on menu item", function() {
equal( logOutput(), "click,(1,2),afterclick,(2,1),(3,3),(1,2)", "Click order not valid." );
});

test( "hover over a menu item with no sub-menu should close open menu", function() {
expect( 2 );

var element = $( "#bar1" ).menubar();
Copy link
Member

Choose a reason for hiding this comment

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

Only one var statement per function.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I wonder if this was JSHinted using grunt, as that should have caught the multiple vars.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Why is that a convention, for my edification. I personally like it but have been unable to influence others at work to adopt it, so I'm curious
  2. I did not use grunt, i just use jslint in my editor. I didn't see anything about running grunt in the README. Ought it be?

Copy link
Member

Choose a reason for hiding this comment

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

This was recently debated actually. The most important reason that I know of is to make the scope clear, but that would apply to many var statements at the top of the function as well.

As for grunt, you can run grunt or run the unit tests, both will lint the source files. It sounds like your editor isn't reading in our .jshintrc file.

Copy link
Member

Choose a reason for hiding this comment

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

  1. I think onevar is a personal pref of the project. The rule really is "it doesn't what the style is, as long as its consistent" :) we've decided on it, and as such everyone must follow it :P. it's just that simple
  2. If grunt isn't mentioned in the contribution guide, we should add it, @scottgonzalez

Copy link
Member

Choose a reason for hiding this comment

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

@mikesherov It's cute that you think we've gotten around to adding a contributing.md :-P I sense one coming from you very soon...

Copy link
Author

Choose a reason for hiding this comment

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

@scottgonzalez @mikesherov : is there an editor that supports directory-specific jshint rules? I'm a Vim guy myself.

Copy link
Member

Choose a reason for hiding this comment

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

Sublime Text 2 has a ton of plugins. One is to run JSHint and look for directory level .jshintrc files. That's what @scottgonzalez and I use.

Copy link
Member

Choose a reason for hiding this comment

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

@wycats Can you point @sgharms at a vim plugin that will find and use the correct .jshintrc based on the current file?

var links = $("#bar1 > li a");
Copy link
Member

Choose a reason for hiding this comment

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

spacing between parens and quotes


var menuItemWithDropdown = links.eq(1);
Copy link
Member

Choose a reason for hiding this comment

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

Don't align operators.

var menuItemWithoutDropdown = links.eq(0);

menuItemWithDropdown.trigger('click', {});
Copy link
Member

Choose a reason for hiding this comment

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

double quotes, spacing, no second argument

Copy link
Member

Choose a reason for hiding this comment

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

Same for all calls to .trigger().

Copy link
Author

Choose a reason for hiding this comment

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

Is there a reason to prefer simulate to trigger?

Copy link
Member

Choose a reason for hiding this comment

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

.simulate() is a higher-level concept, which is meant to come closer to real user interaction.

Copy link
Member

Choose a reason for hiding this comment

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

@scottgonzalez, should I be using .simulate in tests instead of .click() as a general rule?

Copy link
Member

Choose a reason for hiding this comment

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

@mikesherov We should probably be using .simulate() for all events, but in practice we generally use it as needed. At some point I'd like to finally improve simulate, maybe I'll make that a priority for the 1.11 release. We can go down the road of updating all tests to only use .simulate() and then start enforcing it.

menuItemWithoutDropdown.trigger('mouseenter', {});

equal( $(".ui-menu:visible").length, 0 );
Copy link
Member

Choose a reason for hiding this comment

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

Add a message as the third parameter, so that the failure output is clear. Same for assertion below


menuItemWithDropdown.trigger('click', {});
menuItemWithoutDropdown.trigger('click', {});

equal( $(".ui-menu:visible").length, 0 );

});

})( jQuery );
15 changes: 9 additions & 6 deletions ui/jquery.ui.menubar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,15 @@ $.widget( "ui.menubar", {
input.removeClass( "ui-button-text-only" ).addClass( "ui-button-text-icon-secondary" );
}
} else {
// TODO use _on
input.bind( "click.menubar mouseenter.menubar", function( event ) {
if ( ( that.open && event.type === "mouseenter" ) || event.type === "click" ) {
that._close();
}
});
that._on( input, {
click: function( event ) {
if ( that.open ){ that._close(); }
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't being checked for that.open previously, but you're checking now. Is that the correct behavior?

Copy link
Author

Choose a reason for hiding this comment

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

@mikesherov It seems to work the same (mouseenter is fired before the click) but I can see the wisdom in making the fewest required changes. Will change.

},

mouseenter: function( event ) {
Copy link
Member

Choose a reason for hiding this comment

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

There are two spaces after the colon for some reason (happened above too).

Copy link
Author

Choose a reason for hiding this comment

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

Simple, I double space after :

Copy link
Member

Choose a reason for hiding this comment

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

@sgharms, @scottgonzalez meant you shouldn't have two spaces there.

if ( that.open ){ that._close(); }
Copy link
Member

Choose a reason for hiding this comment

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

if ( this.open ) {
    this._close();
}

Copy link
Member

Choose a reason for hiding this comment

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

See above code, use this, not that; also spacing

}
});
}

input
Expand Down