r86625 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86624‎ | r86625 | r86626 >
Date:14:39, 21 April 2011
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
(bug 27833) CollapsibleTabs doesn't collapse tabs due to JS errors
* Change remove() to detach() so .data() isn't lost
* Be more careful using the parent's .data() in addData()
* Change ele (which mysteriously isn't available in the closure) to this, they're the same anyway
Modified paths:
  • /trunk/extensions/Vector/modules/ext.vector.collapsibleTabs.js (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.collapsibleTabs.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.collapsibleTabs.js
——@@ -49,12 +49,14 @@
5050 },
5151 addData: function( $collapsible ) {
5252 var $settings = $collapsible.parent().data( 'collapsibleTabsSettings' );
53 - $collapsible.data( 'collapsibleTabsSettings', {
54 - 'expandedContainer': $settings.expandedContainer,
55 - 'collapsedContainer': $settings.collapsedContainer,
56 - 'expandedWidth': $collapsible.width(),
57 - 'prevElement': $collapsible.prev()
58 - } );
 53+ if ( typeof $settings != 'undefined' ) {
 54+ $collapsible.data( 'collapsibleTabsSettings', {
 55+ 'expandedContainer': $settings.expandedContainer,
 56+ 'collapsedContainer': $settings.collapsedContainer,
 57+ 'expandedWidth': $collapsible.width(),
 58+ 'prevElement': $collapsible.prev()
 59+ } );
 60+ }
5961 },
6062 getSettings: function( $collapsible ) {
6163 var $settings = $collapsible.data( 'collapsibleTabsSettings' );
——@@ -95,7 +97,7 @@
9698 var dataExp = $.collapsibleTabs.getSettings( data.expandedContainer );
9799 dataExp.shifting = true;
98100 $moving
99 - .remove()
 101+ .detach()
100102 .prependTo( data.collapsedContainer )
101103 .data( 'collapsibleTabsSettings', data );
102104 dataExp.shifting = false;
——@@ -107,7 +109,7 @@
108110 var dataExp = $.collapsibleTabs.getSettings( data.expandedContainer );
109111 dataExp.shifting = true;
110112 // remove this element from where it's at and put it in the dropdown menu
111 - $moving.remove().insertAfter( data.prevElement ).data( 'collapsibleTabsSettings', data );
 113+ $moving.detach().insertAfter( data.prevElement ).data( 'collapsibleTabsSettings', data );
112114 dataExp.shifting = false;
113115 $.collapsibleTabs.handleResize();
114116 }
Index: trunk/extensions/Vector/modules/ext.vector.collapsibleTabs.js
——@@ -28,7 +28,7 @@
2929 $( this ).hide();
3030 // add the placeholder
3131 $( '<span class="placeholder" style="display:none;"></span>' ).insertAfter( this );
32 - $( this ).remove().prependTo( target ).data( 'collapsibleTabsSettings', data );
 32+ $( this ).detach().prependTo( target ).data( 'collapsibleTabsSettings', data );
3333 $( this ).attr( 'style', 'display:list-item;' );
3434 //$.collapsibleTabs.getSettings( $( $.collapsibleTabs.getSettings( $( ele ) ).expandedContainer ) )
3535 // .shifting = false;
——@@ -65,13 +65,13 @@
6666 var $target = $( data.expandedContainer ).find( 'span.placeholder:first' );
6767 var expandedWidth = data.expandedWidth;
6868 $moving.css( "position", "relative" ).css( ( rtl ? 'right' : 'left' ), 0 ).css( 'width', '1px' );
69 - $target.replaceWith( $moving.remove().css( 'width', '1px' ).data( 'collapsibleTabsSettings', data )
70 - .animate( { width: expandedWidth+"px" }, "normal", function() {
 69+ $target.replaceWith( $moving.detach().css( 'width', '1px' ).data( 'collapsibleTabsSettings', data )
 70+ .animate( { width: expandedWidth+"px" }, "normal", function( ) {
7171 $( this ).attr( 'style', 'display:block;' );
7272 //$.collapsibleTabs.getSettings( $( $.collapsibleTabs.getSettings( $( ele ) ).expandedContainer ) )
7373 // .shifting = false;
7474 // Do the above, except with guards for JS errors
75 - var data = $.collapsibleTabs.getSettings( $( ele ) );
 75+ var data = $.collapsibleTabs.getSettings( $( this ) );
7676 if ( !data ) {
7777 return;
7878 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r86841Followup r86625: per CR, use loose comparison against null instead of typeof ...catrope10:17, 25 April 2011
r87079MFT r86625 through r87078awjrichards15:02, 28 April 2011
r871701.17: MFT r85546, r86450, r86625, r86805, r86841, r86904, r86973, r87030catrope16:57, 30 April 2011
r873531.17wmf1: MFT r85546, r86409, r86450, r86625, r86788, r86805, r86841, r86904,...catrope20:27, 3 May 2011

Comments

#Comment by Krinkle (talk | contribs)   10:14, 23 April 2011 
+			if ( typeof $settings != 'undefined' ) {

I'm not sure this works as expected. jQuery's .data() returns null when nothig could be found (atleast in the version we use in MediaWiki).

null has a type of "object" not "undefined".

To be future proof (since jQuery 1.5 returns undefined for unset data keys), you can do a loose comparison to null. For some reason doing a loose comparison to null will return true as well when the variable is undefined (this method is used in jQuery's internals as well). 

if ( varThatIsNullOrUndefined == null ) {
 alert(varThatIsNullOrUndefined == null);
}


The only advantage to using typeof is to avoid a ReferenceError if the entire variable was undeclared, but that's not the case since the variable is declared right above it.
#Comment by Catrope (talk | contribs)   10:15, 25 April 2011

Hmm I swear it returned undefined for me. I'll do the loose comparison thing then.

#Comment by Krinkle (talk | contribs)   22:44, 25 April 2011

I checked in Firefox and returns null there as well.

However I did this on mediawiki.org (jQuery 1.4.2), not /trunk, and it returned null. On translatewiki.net (jQuery 1.4.4) it does indeed return undefined.

So I guess this changed in 1.4.4 rather than 1.5. Anyway, it might change in the future as well. Not sure why they changed it.

Status & tagging log