Skip to content
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

Button: Fixing handling of the disabled options on refresh method calls.... #832

Closed
wants to merge 2 commits into from

Conversation

tjvantoll
Copy link
Member

... Fixed #8828 - Button: Refresh does not re-enable disabled button.

See http://bugs.jqueryui.com/ticket/8828.

…ls. Fixed #8828 - Button: Refresh does not re-enable disabled button.
@scottgonzalez
Copy link
Member

I think it'd be better to check for elements which support the disabled property, rather than elements which don't. Wouldn't the current implementation fail for all other elements, such as span and div anyway (not that we encourage it, but we support it). Something like "(is(input or button) and disabled) or class"

@scottgonzalez
Copy link
Member

I guess the logical or was the problem before though, so keep the ternary, but change to checking against input/button?

…arbitrary elements that do not support the disabled property (div, span, etc).
@tjvantoll
Copy link
Member Author

@scottgonzalez Good call. I flipped the call and added a test with a <div>.

@@ -284,7 +284,9 @@ $.widget( "ui.button", {
},

refresh: function() {
var isDisabled = this.element.is( ":disabled" ) || this.element.hasClass( "ui-button-disabled" );
//See #8237 & #8828
Copy link
Member

Choose a reason for hiding this comment

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

being super picky here, but comments should have a space: // See #8237 & #8828

@mikesherov
Copy link
Member

besides my minor not, which I can fix while merging, this looks good to me. Let me know @scottgonzalez, and I can land this.

@mikesherov
Copy link
Member

Thanks, landed in 93abe02

@mikesherov mikesherov closed this Nov 22, 2012
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.

None yet

3 participants