-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Replaced toggleClass with add/removeClass where appropriate #3689
Conversation
It looks like this is implementing 'option A' from #3338, which is the most verbose of all the approaches. Any interest in looking into the cleaner options B or C? |
i did look at B and C. For me B seems the best for compatibility but i thought if this is decided a little later we could first merge this and maybe after that still change those places to use an utility function. |
I think it would be a lot easier to do that cleanup now, rather than going through later and trying to identify addClass()/removeClass() pairs to migrate over. |
You are right, actually it would be fairly easy because all show up in this requests diff, but if you think it's better then i'll just add an utility function. |
@peterflynn i added an util function, so i basically implemented option B. |
@@ -698,7 +699,7 @@ define(function (require, exports, module) { | |||
} | |||
}); | |||
} else { | |||
$(_getHTMLMenuItem(this.id)).toggleClass("disabled", !this._command.getEnabled()); | |||
ViewUtils.toggleClass($(_getHTMLMenuItem(this.id)), "disabled", this._command.getEnabled()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition got inverted
@WebsiteDeveloper done reviewing |
@peterflynn fixes pushed. |
Thanks @WebsiteDeveloper! Merging now. |
Replaced toggleClass with add/removeClass where appropriate
Partial fix for #3338