Skip to content

Commit 374661a

Browse files
committed
Accordion: Code review.
1 parent 3e8ec7e commit 374661a

File tree

1 file changed

+13
-9
lines changed

1 file changed

+13
-9
lines changed

ui/jquery.ui.accordion.js

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,7 @@ $.widget( "ui.accordion", {
5656
}
5757
this.active = this._findActive( options.active )
5858
.addClass( "ui-accordion-header-active ui-state-active" )
59-
.toggleClass( "ui-corner-all" )
60-
.toggleClass( "ui-corner-top" );
59+
.toggleClass( "ui-corner-all ui-corner-top" )
6160
this.active.next().addClass( "ui-accordion-content-active" );
6261

6362
this._createIcons();
@@ -69,13 +68,15 @@ $.widget( "ui.accordion", {
6968

7069
this.headers
7170
.attr( "role", "tab" )
71+
// TODO: use _bind()
7272
.bind( "keydown.accordion", $.proxy( this, "_keydown" ) )
7373
.next()
7474
.attr( "role", "tabpanel" );
7575

7676
this.headers
7777
.not( this.active )
7878
.attr({
79+
// TODO: document support for each of these
7980
"aria-expanded": "false",
8081
"aria-selected": "false",
8182
tabIndex: -1
@@ -137,9 +138,7 @@ $.widget( "ui.accordion", {
137138
.removeAttr( "role" )
138139
.removeAttr( "aria-expanded" )
139140
.removeAttr( "aria-selected" )
140-
.removeAttr( "tabIndex" )
141-
.find( "a" )
142-
.removeAttr( "tabIndex" )
141+
.removeAttr( "tabIndex" );
143142
this._destroyIcons();
144143

145144
// clean up content panels
@@ -162,6 +161,7 @@ $.widget( "ui.accordion", {
162161

163162
if ( key === "event" ) {
164163
if ( this.options.event ) {
164+
// TODO: this is incorrect for multiple events (see _setupEvents)
165165
this.headers.unbind( this.options.event + ".accordion", this._eventHandler );
166166
}
167167
this._setupEvents( value );
@@ -184,12 +184,15 @@ $.widget( "ui.accordion", {
184184
// #5332 - opacity doesn't cascade to positioned elements in IE
185185
// so we need to add the disabled class to the headers and panels
186186
if ( key === "disabled" ) {
187-
this.headers.add(this.headers.next())
187+
this.headers.add( this.headers.next() )
188+
// TODO: why do we have an accordion-specific disabled class?
189+
// widget-specific classes seem to exist in a lot of plugins
188190
.toggleClass( "ui-accordion-disabled ui-state-disabled", !!value );
189191
}
190192
},
191193

192194
_keydown: function( event ) {
195+
// TODO: remove disabled check when using _bind()
193196
if ( this.options.disabled || event.altKey || event.ctrlKey ) {
194197
return;
195198
}
@@ -300,6 +303,7 @@ $.widget( "ui.accordion", {
300303

301304
_setupEvents: function( event ) {
302305
if ( event ) {
306+
// TODO: use _bind()
303307
this.headers.bind( event.split( " " ).join( ".accordion " ) + ".accordion",
304308
$.proxy( this, "_eventHandler" ) );
305309
}
@@ -377,7 +381,7 @@ $.widget( "ui.accordion", {
377381
} else {
378382
toHide.hide();
379383
toShow.show();
380-
this._completed( data );
384+
this._toggleComplete( data );
381385
}
382386

383387
// TODO assert that the blur and focus triggers are really necessary, remove otherwise
@@ -406,7 +410,7 @@ $.widget( "ui.accordion", {
406410
options = down && animate.down || animate,
407411
complete = function() {
408412
toShow.removeData( "ui-accordion-height" );
409-
that._completed( data );
413+
that._toggleComplete( data );
410414
};
411415

412416
if ( typeof options === "number" ) {
@@ -438,7 +442,7 @@ $.widget( "ui.accordion", {
438442
duration, easing, complete );
439443
},
440444

441-
_completed: function( data ) {
445+
_toggleComplete: function( data ) {
442446
var toShow = data.newContent,
443447
toHide = data.oldContent;
444448

0 commit comments

Comments
 (0)