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

Already on GitHub? Sign in to your account

Menu: Added autoCollapse as the default #440

Merged
merged 1 commit into from Sep 12, 2011

Conversation

Projects
None yet
2 participants
Owner

kborchers commented Aug 19, 2011

Menu: Added autoCollapse as the default

@jzaefferer jzaefferer commented on an outdated diff Aug 21, 2011

ui/jquery.ui.menu.js
@@ -250,6 +250,12 @@ $.widget( "ui.menu", {
.prepend( '<span class="ui-menu-icon ui-icon ui-icon-carat-1-e"></span>' );
menu.attr( "aria-labelledby", item.attr( "id" ) );
});
+
+ submenus.add( this.element ).bind( "mouseleave.menu", function( event ) {
@jzaefferer

jzaefferer Aug 21, 2011

Owner

Would be nice if we could use event delegation here as well, doing the binding just once in _create. Should then also use _bind.

@jzaefferer jzaefferer commented on an outdated diff Aug 21, 2011

ui/jquery.ui.menu.js
@@ -346,21 +352,21 @@ $.widget( "ui.menu", {
},
collapseAll: function( event ) {
- this.element
- .find( "ul" )
- .hide()
- .attr( "aria-hidden", "true" )
- .attr( "aria-expanded", "false" )
- .end()
- .find( "a.ui-state-active" )
- .removeClass( "ui-state-active" );
+ var currentMenu = event ? $( event.target ).is( ".ui-menu" ) ? $( event.target ) : $( event.target ).closest( ".ui-menu" ).length ? $( event.target ).closest( ".ui-menu" ) : false : false;
@jzaefferer

jzaefferer Aug 21, 2011

Owner

This line needs some refactoring or at least formatting. Too many ternaries. Too much duplicated, e.g. this happens three times: $( event.target ).closest( ".ui-menu" )

Owner

jzaefferer commented Aug 21, 2011

Works really well for the menubar, gets us pretty close to the behaviour of the OSX menubar.

Some code issues still need to be resolved, see above.

Also: Can we add unit tests for this?

Owner

kborchers commented Aug 22, 2011

@jzaefferer Made modifications and added a unit test. Still getting the hang of QUnit so let me know if anything needs to change there or if you have any thoughts on my changes based on your comments.

@jzaefferer jzaefferer commented on an outdated diff Sep 11, 2011

ui/jquery.ui.menu.js
@@ -45,6 +45,10 @@ $.widget( "ui.menu", {
event.preventDefault();
}
});
+ var mouseleaveHandler = function( event ) {
@jzaefferer

jzaefferer Sep 11, 2011

Owner

Should move this on the prototype, e.g. as _mouseleave, and use this instead of self. this points to the instance when using _bind.

Owner

kborchers commented Sep 12, 2011

@jzaefferer How's that look?

@jzaefferer jzaefferer merged commit 94317d7 into jquery:master Sep 12, 2011

Owner

jzaefferer commented Sep 12, 2011

Looking good. Just landed it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment