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: Add support for structures other than UL/LI #442

Merged
merged 1 commit into from Sep 19, 2011

Conversation

Projects
None yet
2 participants
Owner

kborchers commented Aug 20, 2011

Menu: Add support for structures other than UL/LI

@jzaefferer jzaefferer and 1 other commented on an outdated diff Aug 20, 2011

tests/visual/menu/menu.html
@@ -173,6 +184,87 @@
<li><a href="#">Amesville</a></li>
</ul>
+<div id="menu5">
+ <h3><a href="#">Aberdeen</a></h3>
@jzaefferer

jzaefferer Aug 20, 2011

Owner

So we're sticking with nesting a-elements? Did you investigate that?

@kborchers

kborchers Aug 20, 2011

Owner

I did start looking into it and figured we could address that separately. There is a lot of code/styles that depend on the a elements being there so figured we could get other structures in, then figure out the best way to remove a's.

Do you have any feelings one way or the other on removing the a elements? I feel like it would be nice to remove that requirement for devs but at the same time it is nice knowing that will be there for some of our selectors. Specifically, in my input menu extension, I use the fact that there isn't an a to determine a group label for grouping menu items.

Owner

jzaefferer commented Sep 13, 2011

My refactorings yesterday messed this one up a good bit, sorry 'bout that. Could you look into merging it?

@jzaefferer jzaefferer and 1 other commented on an outdated diff Sep 13, 2011

tests/visual/menu/menu.html
function create() {
menus.menu({
select: function(event, ui) {
$("<div/>").text("Selected: " + ui.item.text()).appendTo("#log");
}
});
+
+ $("#menu6").menu({
+ select: function(event, ui) {
+ $("<div/>").text("Selected: " + ui.item.text()).appendTo("#log");
+ },
+ items: ".menuItemElement"
@jzaefferer

jzaefferer Sep 13, 2011

Owner

Wouldn't this also work with the default for the items option? If so, would be good to have a usecase as demo where the items option is really needed. Like a menu that actually has a bunch of non-menuitem content.

@kborchers

kborchers Sep 13, 2011

Owner

This example does have non-menu-item content inside of the items. It will not work with the default because both the menus and menu items are divs which causes issues when creating the submenus. I have also made changes in the upcoming updated commit so that items represents menus instead of each of the menu items so hopefully that will work better.

@jzaefferer jzaefferer and 1 other commented on an outdated diff Sep 13, 2011

ui/jquery.ui.menu.js
@@ -220,19 +221,20 @@ $.widget( "ui.menu", {
refresh: function() {
var self = this,
+ menuElement = this.element[0].tagName.toLowerCase(),
@jzaefferer

jzaefferer Sep 13, 2011

Owner

This seems rather hackish. Can we just remove the selector part that requires this? Or do we actually need another option for this, ala menuItems: "ul" (as default).

@kborchers

kborchers Sep 13, 2011

Owner

Agreed. You will see in the updated commit that I decided to have items be the menus, not the items. Then I can select children from the menus. Seems to work better without that hacky solution.

@jzaefferer jzaefferer and 1 other commented on an outdated diff Sep 13, 2011

ui/jquery.ui.menu.js
- // initialize nested menus
- submenus = this.element.find( "ul:not(.ui-menu)" )
- .addClass( "ui-menu ui-widget ui-widget-content ui-corner-all" )
- .attr( "role", "menu" )
- .hide()
- .attr( "aria-hidden", "true" )
- .attr( "aria-expanded", "false" ),
+ // initialize nested menus
+ submenus = this.element.find( menuElement + ":not(.ui-menu, " + this.options.items + ")" )
@jzaefferer

jzaefferer Sep 13, 2011

Owner

Why would you select items as submenus?

This also needs to be reflected in the demos - the one with a custom items option has no submenus.

@kborchers

kborchers Sep 13, 2011

Owner

Not sure what you mean, that is selecting submenus not items. Let me know if you still have an issue with this after I update the pull as I have made a number of changes and added in your refactoring.

@jzaefferer jzaefferer and 1 other commented on an outdated diff Sep 13, 2011

ui/jquery.ui.menu.js
@@ -371,11 +373,11 @@ $.widget( "ui.menu", {
},
collapse: function( event ) {
- var newItem = this.active && this.active.parents("li:not(.ui-menubar-item)").first();
+ var newItem = this.active && this.active.parents( this.options.items + ":not(.ui-menubar-item)" ).first();
@jzaefferer

jzaefferer Sep 13, 2011

Owner

Can we use closest here? Optionally with this.element has the second context argument: http://api.jquery.com/closest#closest2

@jzaefferer

jzaefferer Sep 13, 2011

Owner

Also, what is going on here with .ui-menubar.item? Seems like that doesn't really belong here...

@kborchers

kborchers Sep 13, 2011

Owner

Yes, will use closest.

Not sure on the menubar thing. That was there when I started working ... from Hans I think. I removed it.

Owner

jzaefferer commented Sep 13, 2011

Finally did an actual code review. Remaining changes look good. We really need to automate whitspace formatting...

Owner

kborchers commented Sep 13, 2011

Updated commit coming shortly

Owner

jzaefferer commented Sep 13, 2011

I'm wondering how much more flexiblity this get us if you can only customize what the menu elements consist of, while menu items are still determined by having to be a child element with a containing anchor element.

The changes are mostly minimal now, so I'm fine with landing this, then gather more feedback.

Before that, can you fix the styling on the last menu on the visual test page? There's the infamous 1px jumping when first hovering the bigger menu items, jumps back on 'blur'.

Owner

jzaefferer commented Sep 13, 2011

Chatted a bit with Richard about the design. Let's fix the demo styling, then we can land this.

Owner

kborchers commented Sep 13, 2011

I fixed the jump. I had the css in the demo remove the border between the elements on hover which was just for looks and not important. That bottom border is not added via menu, just the css in the demo.

Owner

jzaefferer commented Sep 14, 2011

Okay, getting close.

Do we need the class="menuItemElement" anymore? Seems like that's neither used by JS nor CSS.

Can you check the menu unit tests and add the default for the items option? Better yet, add a unit test for a custom items option.

Owner

kborchers commented Sep 14, 2011

Oh, shoot! No, we don't need that class anymore. I will pull that out and look into unit tests when I get into work.

@jzaefferer jzaefferer merged commit 9e617bb into jquery:master Sep 19, 2011

Owner

jzaefferer commented Sep 19, 2011

Landed! Highfive!

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