r105613 MediaWiki - Code Review archive

+			$(document).ready( function() {
+				$.ajax( {
+					'type': 'POST',
+					'url': mw.util.wikiScript( 'api' ),
+					'data': query,
+					'success': function (data) {
+						mb.userData = data.query.userinfo;
+					},
+					'error': function( jqXHR, textStatus, errorThrown ) {
+						mb.userData = null;
+					},
+					'dataType': 'json'
+				} );
+			});

Why is this AJAX call in a document ready wrapper? I mean I'm sure it's har

+			$(document).ready( function() {
+				$.ajax( {
+					'type': 'POST',
+					'url': mw.util.wikiScript( 'api' ),
+					'data': query,
+					'success': function (data) {
+						mb.userData = data.query.userinfo;
+					},
+					'error': function( jqXHR, textStatus, errorThrown ) {
+						mb.userData = null;
+					},
+					'dataType': 'json'
+				} );
+			});

Why is this AJAX call in a document ready wrapper? I mean I'm sure it's harmless, but it looks strange to me.

+	'moodbar-tooltip-title' => 'Are you sad, happy, or confused about editing Wikipedia?',
+	'moodbar-tooltip-subtitle' => 'Your feedback about editing Wikipedia helps us make the site better.',

Don't hardcode anything Wikipedia specific, or Wikimedia specific in general (Wikimedia has other projects as well, not to mention use by third-parties). Use {{SITENAME}}. This means in JavaScript it will likely need to be replaced manually with a regexp as this is not done by default yet. (a simple .replace( new RegExp( $.escapeRE('MediaWiki'), 'g' ), mw.config.get( 'wgSiteName' ) ); will do the trick.

+		$( '#fbd-filters-type-praise, #fbd-filters-type-confusion, #fbd-filters-type-issues' ).each( function() {
+			$(this).prop( 'checked', true);
+		});

No need for .each(). All jQuery methods (except for very few) always run on the entire jQuery collection. No need to create a new jQuery collection for this and then run .prop()( when loops over the single-item collection). The following is how jQuery prototype is intended:

	$( '#fbd-filters-type-praise, #fbd-filters-type-confusion, #fbd-filters-type-issues' ).prop( 'checked', true);

+				mb.ui.overlay.find( '.mw-moodBar-emailSubmit').removeAttr('disabled');
+			} else {
+				mb.ui.overlay.find( '.mw-moodBar-emailSubmit').attr({'disabled':'true'});		

Initial states are set by the "disabled" attribute, but the interactive state should be triggered by manipulating the property directly, not the attribtue. Use .prop( 'disabled', false ); and .prop( 'disabled', true ); respectively (which sets element.disabled = ..; property, as supposed element.setAttribute( 'disabled', .. );

+++ trunk/extensions/MoodBar/MoodBar.php	(revision 105613)
@@ -91,6 +91,21 @@
 	),
 );
 
+$wgResourceModules['ext.moodBar.tooltip'] = $mbResourceTemplate + array(

+	'dependencies' => array(

+		'jquery.client',
+	),

jQuery.client doesn't appear to be used by ext.moodBar.tooltip.

Roan already touched this area:

+++ trunk/extensions/MoodBar/modules/ext.moodBar/ext.moodBar.init.js	(revision 105613)

 	var mb = mw.moodBar = {

+		userData: {}, //set by getUserInfo

@@ -54,13 +56,40 @@
+			// Assign user props to mb.userData object.
+			mb.getUserInfo();
 		},

+		getUserInfo: function() {
+			var query = {
+				action: 'query',
+				meta: 'userinfo',
+				uiprop: 'email',
+				format: 'json'
+			};
+			$(document).ready( function() {
+				$.ajax( {
+					'type': 'POST',
+					'url': mw.util.wikiScript( 'api' ),
+					'data': query,
+					'success': function (data) {
+						mb.userData = data.query.userinfo;
+					},
+					'error': function( jqXHR, textStatus, errorThrown ) {
+						mb.userData = null;
+					},
+					'dataType': 'json'
+				} );
+			});
+			

ApiQueryUserInfo is't a POST module. and userData appears unused. Wherever it would be used, it would probably risk being not defined yet. Since getuserInfo is delayed asynchronously by both document-ready and the AJAX-request, it's hard impossibly to reliably use mw.moodBar.userData.

Should probably take a callback as argument and cache it in a local variable for immediate callback-calling if already fetched.

Thanks for the review. I've followed up with r105613 which addresses most of this.

All remaining items addressed. Code is deployed. Please follow ups.