User:Krinkle/Extension review/WikiLove

WikiLove extension review by Krinkle, as of r88383.

Directory structure

• Modules are should be stored in a /modules folder, like:
  • /extensions/WikiLove/modules/
    • ext.wikiLove/
      • ext.wikiLove.js
      • ext.wikiLove.css
      • ext.wikiLove.defaultTypes.js
    • jquery.elastic/
      • jquery.elastic.js

Code style

The PHP coding looks good and in harmony with our conventions. Not much to say about that. The JavaScript coding style looks mostly okay although some deprecated methods are used (accessing legacy globals for instance) and some whitespacing around if ( ) { } statements. Check out RL/JD, Code_conventions#JavaScript and Code_conventions#jQuery out to be sure.

WikiLove.js

options

Right now all these are publicly available as object members of $.wikiLove, I'm not sure which are supposed to be editable by local wikis and which not, but I suggest putting actual (supposed-to-be editable) configuration options in an "options" object to make it easier for users to understand what they can/should edit. This also makes it possible to later (see MediaWiki:WikiLove.js) limit the hook to only only these options (and not the rest of the $.wikiLove object, ie. overwriting $.wikiLove.openDialog or something crazy like that).

MediaWiki:WikiLove.js

Loading through mw.loader.load isn't synchronous per se, if later code depends on it (like overriding configuration variables) it should be loaded with a callback to the rest of the code. This could be done through jQuery.getScript, however since it's not an external script, it might as well be registered as part of a module. (ie. ext.wikiLove.local) to get nice caching, combining and minification. This would be a ResourceLoaderWikiModule based module loading MediaWiki:WikiLove.js). The callback can then be passed to mw.loader.using( 'ext.wikiLove.local', fn );

Alternatively, since it looks like MediaWiki:WikiLove.js is unconditionally loaded, you may want to do this more server side to avoid making an HTTP request half-way $.wikiLove.openDialog:

• Register jquery.elastic (jquery.elastic.js)
• Register ext.wikiLove.startup (ext.wikiLove.core.js, ext.wikiLove.css and ext.wikiLove.defaultTypes.js, dependencies: jquery.elastic).
  - ext.wikiLove.core.js would only define the $.wikiLove object and not 'do' anything yet.
  - ext.wikiLove.core.js would contain something like var options = { some: 'defaults' }; and $.wikiLove.optionHook = function(){ };
• Register ext.wikiLove.local (ResourceLoaderWikiModule: MediaWiki:WikiLove.js, dependencies: ext.wikiLove.startup)
  This wiki page may (if local wiki decides to) contain $.wikiLove.optionsHook = function( opts ){ opts.some = 'custom'; return true; };.
• Register ext.wikiLove.init (ext.wikiLove.init.js, depending on ext.wikiLove.local).
  This file would call$.wikiLove.optionsHook( options );, define $.wikiLove.init and pass $.wikiLove.init to jQuery's document ready hook.

Then only addModule ext.wikiLove.init to output, the rest will be taken care of by ResourceLoader and all still in a single load.php request.

wgExtensionAssetsPath

Referring to static files (like images) should be done through wgExtensionAssetsPath (not through wgScriptPath + '/extensions/'). Some wikis override this path (such as Wikimedia). But also many users that run a wiki off trunk, that have /trunk/extensions and /trunk/phase3 and alias the former to /trunk/phase3/extensions/my-symbolic-link), who set wgExtensionAssetsPath to that. In those cases wgScriptPath + '/extensions/WikiLove' would be a 404 error.

Interface images

I see several html elements containing a single ‎<img> which I think may be better done through CSS with classes and background images. This also enables ResourceLoader to convert them (when indicated with /* embed */) to base64 which saves a few extra HTTP requests.

Building HTML

There's a lot of DOM manipulation going on. Take a look at jquery.articleFeedback.js to see how this was done in ArticleFeedback (one main HTML template string with <html:msg> placeholders, then the HTML string is passed to jQuery, then some additional manipulation is done, and finally $.fn.localize() to solve the placeholders at once).

Selectors

jQuery's Sizzle engine (like the browsers native CSS engine) goes from right to left. $( '#wlTypes a' ) gets all Anchor tags on the page, and the parent is checked of each to match #wlTypes. Aside from that, jQuery has a quickExpr detector. Whatever passes that regular expression will be taken care of without initializing the Sizzle engine. A simple ID-selector is one of those quickExpr-things. The following would be many times faster due the above two things (passing quickExpr and getting only the anchor tags within context of the ID-tag):

- $( '#wlTypes a' );
+ $( '#wlTypes' ).find( 'a' );

preventDefault

Returning false from a callback function (submitSend, submitPreview) should not be done – unless done for reasons other than preventing default, such as stopping event bubbling (stopPropagation) or disallowing local wikis to bind events to it. The "e" argument passed to callbacks bound through jQuery is a normalized version of the Event object (see jQuery Event Object) so preventDefault is always available and will internally fallback to emulated behaviour.

Storage & settings (local & public)

When looking back and forth where members are coming from I get the impression currentTypeId (and others alike) may be better suited as local variables within the IIFE rather than (publicly settable and writable) object members. This will also give a slight performance boost as accessing (deep) object members is slower than accessing (local) variables directly:

• (var) currentTypeId vs. (window >) $ > wikiLove > currentSubtypeId

In the following code a rather deep member is accessed multiple times, that should probably be cached:

	var needsHeader = $.inArray( 'header', $.wikiLove.types[$.wikiLove.currentTypeId].fields );

		if( $.wikiLove.types[$.wikiLove.currentTypeId].showNotify ) {
			// ...
		}

	for( var subtypeId in $.wikiLove.types[$.wikiLove.currentTypeId].subtypes ) {
		var subtype = $.wikiLove.types[$.wikiLove.currentTypeId].subtypes[subtypeId];

Like

	var currentType = $.wikiLove.types[$.wikiLove.currentTypeId];

	var needsHeader = $.inArray( 'header', currentType.fields );

		if ( currentType.showNotify ) {
			// ...
		}

	for ( var subtypeId in currentType.subtypes ) {
		var subtype = currentType.subtypes[subtypeId];

ID and Class names

The CSS IDs and classes appear a bit general to me compared to our convention of prefixing. Especially IDs like "wlTypes" and "wikiLoveDialog" could potentially appear in an article's heading and thus as ID on that ‎<h2>. IDs/Classes like mw-wikilove-foobar or mw-ext-wl-foobar are probably safer.

Selecting, chaining caching (jQuery)

Currently for almost every manipulation a new jQuery object is initiated. Multiple manipulations to the same object should be cached and chained for shorter code and faster execution. Also some .find() calls can be merged into the main selector, like $( '#wikiLoveDialog' ).find( '.wlError' ) to $( '#wikiLoveDialog .wlError' );.

• Chaining: $( '#wlHeader' ).val( "" ).show();
• If you need a gap between two actions (a function call, a condition check or what have you), one can still re-use the same object later by storing it in a variable

var $header = $( '#wlHeader' );
$header.foo().bar();
if ( bazIsGood() ) {
  $header.baz();
}

• Due to jQuery's built-in support for group-manipulations (it's optimized for group manipulations), the following can be shortened and speed up:

// Line 278: WikiLovejs
updateAllDetails: function(){

	if( typeof $.wikiLove.currentTypeOrSubtype.gallery == 'object' ) {
		$( '#wlGalleryLabel' ).show();
		$( '#wlGallery' ).show();
		$( '#wlGallerySpinner' ).show();
		$.wikiLove.makeGallery();
	}
	else {
		$( '#wlGalleryLabel' ).hide();
		$( '#wlGallery' ).hide();
		$( '#wlGallerySpinner' ).hide();
	}

};

• By using a comma seperated CSS selector and caching the jQuery object outside the functions scope:

var $gallery = $( '#wlGalleryLabel, #wlGallery, #wlGallerySpinner' );

updateAllDetails = function(){

	if ( typeof options.currentTypeOrSubtype.gallery == 'object' ) {
		$gallery.show();
		$.wikiLove.makeGallery();
	} else {
		$gallery.hide();
	}

};

Syntax and utilities

Although almost nobody does it, and it's rare, when accessing the API we should take wgScriptExtension in to account when accessing scripts.

WikiLove reminded me the need for a utility function (much like wfScript in PHP), so I've added one in r88513: mw.util.wikiScript( 'api' ).

In $.wikiLove.doSend the user is sent to a unescaped location. The title could contain illegal or unsafe characters. In general mw.util.rawurlencode should be used (or jQuery.param). However for page titles there's a few special cases (such as slashes or subpages). Going to "Foo%3A:Bar%2FBaz" (Foo:Bar/Baz) may cause a MediaWiki error in some environments. To avoid that use wfUrlencode in PHP and mw.util.wikiUrlencode in JS when dealing with page titles in URLs.

As of 1.17 a utility function exists that takes care of all this: mw.util.wikiGetlink( 'title' )

WikiLove.css

The stylesheet look OK. I personally like the blue tone of the modal box. Perhaps we can turn this into a jQuery UI theme some day ?

I did notice some small things to clean up tho:

• left-over copy paste (#ca-unwatch.icon, #ca-watch.icon { margin-right: 0px; })
• overly specific rule (#wikiLoveDialog #wlPreview #wlPreviewArea .editsection { .. )

Load order

When loading the page one can see a short flickr when the text "Show appreciation" is hidden in favour of the heart. Although this button nor the contents are modified by JavaScript, the actual style declerations are loaded through JavaScript on the bottom of the page so there's a short delay between PHP outputting the button and ResourceLoader applying the styles (and swapping the text for an image).

Recently a solution for this has been implemented. Modules that affect the view of the above-the-fold directly onload should be in a seperate (mini) module with 'position' set to 'top' in the registry. This mini module would contain all styles and a bit of javascript that calls something like mw.util.addPortletLink.

User interface / visually

As mentioned before I like the custom theme building on jQuery UI. One thing I found weird while testing out locally is that one can only click 'Preview' when initially in the WikiLove dialog, it's not obvious to me that there will be a "Preview" box below and that the submit button will make it's introduction there. Wether or not Preview is mandatory is a seperate question, but I think the user should somehow know what's coming up.