Selectmenu: Introduce _renderButtonItem method #1299
Conversation
| menu = element.selectmenu( "menuWidget" ); | ||
|
|
||
| instance._renderButtonItem = function( buttonItem, item ) { | ||
| buttonItem.parent().data( "test", item.value ); |
There was a problem hiding this comment.
Why is this part of the test?
There was a problem hiding this comment.
I wanted to change the parent node to make sure that possible to. Unnecessary?
There was a problem hiding this comment.
ps: is this the correct place for test like this?
There was a problem hiding this comment.
Very much unnecessary. The extension point is for modifying the item, not the parent. There should be no assumption that you can walk the tree like this. In fact, this makes me change my mind and say that this is not a good implementation. _renderButtonItem() should be generating the item, just like _renderItem() does. That item should then be appended to the button.
There was a problem hiding this comment.
Yes, this is the correct file for the test.
There was a problem hiding this comment.
In fact, this makes me change my mind and say that this is not a good implementation. _renderButtonItem() should be generating the item, just like _renderItem() does. That item should then be appended to the button.
This way we would need to append the element on create and replace the element on each select.
Keep in mind that _renderItems is called once on create or by refresh(). _renderButtonItem is called on create, refresh() and on each item select as we need to update button content.
There was a problem hiding this comment.
This way we would need to append the element on create and replace the element on each select.
Why is that an issue?
There was a problem hiding this comment.
Probably leads to a11y issues as the dom node changes?
There was a problem hiding this comment.
Why? There's nothing to indicate that changing the span would be any different than changing the content of the span.
|
I've changed the implementation so the button item element is created within the _renderButtonItem method. |
|
looks good |
|
I put an example together to see that this would look like: http://jsfiddle.net/tj_vantoll/M6899/. I like it; we'll have to make sure we document these methods well though. I'll work on getting the two existing extension points out there. |
|
For the record, this has to wait for 1.12, so we can't land it right now. |
|
fyi the PRs for selectmenu's extension points are at jquery/api.jqueryui.com#223 and jquery/api.jqueryui.com#226. When this lands those should land those too. |
|
@fnagel Can you update this to handle the new menu item wrappers? |
|
@scottgonzalez Sure. It's already in master or is that feature still a PR? |
|
It's in master. I was going to merge this just now, but it didn't work because of the wrapper update. |
fnagel
force-pushed
the
selectmenu-render-button-final
branch
from
a2a3e2f
to
b8b306d
Compare
|
Rebased onto master. |
| }; | ||
|
|
||
| element.selectmenu( "refresh" ); | ||
| links = menu.find( "li.ui-menu-item" ); |
There was a problem hiding this comment.
This is never used. I'll remove it when I merge.
There was a problem hiding this comment.
Uhh, yeah. Seems I missed that when squashing the commits. Thanks!
Fixes #10142