MediaWiki r67559 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r67558‎ | r67559 (on ViewVC)‎ | r67560 >
Date:19:40, 7 June 2010
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
Added bucket testing for collapsible nav (50% of people get the old version), and also a threshold which makes sure that you don't end up with a "more languages" portal with 1 or 2 links it in. The threshold is set to 3 right now, but it's configurable.
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r67744Solves code style issue in r67559 and removes debugger statement added in r67...tparscal18:54, 9 June 2010

Comments

#Comment by Catrope (talk | contribs)   20:08, 7 June 2010
+		var version = 2;
...
+			var version = $j.cookie( 'vector-nav-pref-version' );
...
+			var version = 1;

I would feel a lot safer if you just did var version; to declare version before going into a complex if-else structure that is now obliged to declare version in every possible code path.

+	if ( wgCollapsibleNavForceNewVersion == true ) {
...
+		if ( wgCollapsibleNavBucketTest == true ) {

== true? Really? What's wrong with just using if ( wgFoo )?

+				console.log( version );

Bad Trevor ;)

+		$primary = $j( '#p-lang ul.primary' );
+		$secondary = $j( '#p-lang ul.secondary' );
...
+			$link = $secondary.find( '.interwiki-' + languages[i] );

Use var $primary = ... so you don't pollute the global scope or accidentally overwrite something that's already there.

+					$link.remove().appendTo( $primary );
...
+			$secondary.remove().appendTo( $j( '#p-lang-more div.body' ) );

My warning against using remove() in the CR comments on r67548 applies here as well.

+			if ( state == 'true' || ( state == null && i < 1 ) || version == 1 && id == 'p-lang' ) {

Please be consistent in parenthesizing here.

Status & tagging log

  • 17:17, 14 June 2010 Catrope (talk | contribs) changed the status of r67559 [removed: fixme added: resolved]
  • 20:08, 7 June 2010 Catrope (talk | contribs) changed the status of r67559 [removed: new added: fixme]