Skip to content

Selectmenu #866

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.

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 409 commits into from
Closed

Selectmenu #866

wants to merge 409 commits into from

Conversation

jzaefferer
Copy link
Member

New PR for selectmenu, since #492 got too long.

See also http://wiki.jqueryui.com/w/page/12138056/Selectmenu

fnagel and others added 30 commits February 26, 2012 01:50
…expects and remove the extra space added to disabled items
… conformance to revision e2a6cdd and in order to make Selectmenu work again
@fnagel
Copy link
Member

fnagel commented May 24, 2013

Any feedback on my latest comments?

@jzaefferer
Copy link
Member Author

@fnagel once you've addressed those four we just discussed, can you close this PR and create a new one, once more? Then we can do a (hopefully) final review there.

@fnagel
Copy link
Member

fnagel commented May 30, 2013

@jzaefferer Yes, I will pen a new one.

There is one thing I mentioned in a comment above:

I noticed an issue in IE8 and IE7: the change event unit test fails. 
The change event wont fire but it fails only in the unit test. 
It works when used by actual mouse / keyboard interaction. Any ideas?

@scottgonzalez
Copy link
Member

Perhaps related to the fact that you're treating .simulate( "focus" ) as synchronous, while focus is always async in IE.

@fnagel
Copy link
Member

fnagel commented May 30, 2013

@scottgonzalez I've changed to asyncTest but that didn't help.

@scottgonzalez
Copy link
Member

We were able to track down the problem to .simulate( "click" ) and delegated events. Can you go through the tests and change all of the instances of .simulate( "click" ) to just .trigger( "click" ) and see if everything continues to pass for you? We'll probably want to write a real click simulation at some point that handles mouse down, up, etc.

@scottgonzalez
Copy link
Member

To remove the looping, you can add something like this inside _move():

if ( this.menu.menu( "isFirstItem" ) && /^previous/.test( direction ) ||
        this.menu.menu( "isLastItem" ) && /^next/.test( direction ) ) {
    return;
}

We'll need tests for this as well.

@fnagel fnagel closed this Jul 1, 2013
@fnagel fnagel mentioned this pull request Jul 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants