Skip to content
Permalink
Browse files

Accordion: Code review.

  • Loading branch information
scottgonzalez committed Mar 2, 2012
1 parent 3e8ec7e commit 374661a2ffaaef93f2b679826bc69c0b214b5310
Showing with 13 additions and 9 deletions.
  1. +13 −9 ui/jquery.ui.accordion.js
@@ -56,8 +56,7 @@ $.widget( "ui.accordion", {
}
this.active = this._findActive( options.active )
.addClass( "ui-accordion-header-active ui-state-active" )
.toggleClass( "ui-corner-all" )
.toggleClass( "ui-corner-top" );
.toggleClass( "ui-corner-all ui-corner-top" )

This comment has been minimized.

Copy link
@dmethvin

dmethvin Mar 7, 2012

Member

Moar ;

The .toggleClass() semantics changed a bit over the years; up until I think 1.4 it would toggle all classes ON if any of the classes were there. Now it correctly toggles them independently. Dunno if that affects the code here, depends on whether these values are in sync and which versions of jQuery core you are supporting. http://jsfiddle.net/dmethvin/7GcQ6/

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Mar 7, 2012

Author Member

Yup, it was split for old jQuery support, but 1.9 will only support 1.6+, so we're good.

Nice catch on the missing ;.

this.active.next().addClass( "ui-accordion-content-active" );

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

this.headers
.attr( "role", "tab" )
// TODO: use _bind()
.bind( "keydown.accordion", $.proxy( this, "_keydown" ) )
.next()
.attr( "role", "tabpanel" );

this.headers
.not( this.active )
.attr({
// TODO: document support for each of these

This comment has been minimized.

Copy link
@jzaefferer

jzaefferer Mar 7, 2012

Member

As inline comments?

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Mar 7, 2012

Author Member

Either inline comments or on the planning wiki. We just need to know why we're doing both instead of just the standard one.

"aria-expanded": "false",
"aria-selected": "false",
tabIndex: -1
@@ -137,9 +138,7 @@ $.widget( "ui.accordion", {
.removeAttr( "role" )
.removeAttr( "aria-expanded" )
.removeAttr( "aria-selected" )
.removeAttr( "tabIndex" )
.find( "a" )
.removeAttr( "tabIndex" )
.removeAttr( "tabIndex" );
this._destroyIcons();

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

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

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

_setupEvents: function( event ) {
if ( event ) {
// TODO: use _bind()
this.headers.bind( event.split( " " ).join( ".accordion " ) + ".accordion",
$.proxy( this, "_eventHandler" ) );
}
@@ -377,7 +381,7 @@ $.widget( "ui.accordion", {
} else {
toHide.hide();
toShow.show();
this._completed( data );
this._toggleComplete( data );
}

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

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

_completed: function( data ) {
_toggleComplete: function( data ) {
var toShow = data.newContent,
toHide = data.oldContent;

0 comments on commit 374661a

Please sign in to comment.
You can’t perform that action at this time.