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  statements. Check out RL/JD, Code_conventions and Code_conventions out to be sure.

options
Right now all these are publicly available as object members of, 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  object, ie. overwriting   or something crazy like that).

MediaWiki:WikiLove.js
Loading through  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, however since it's not an external script, it might as well be registered as part of a module. (ie. ) to get nice caching, combining and minification. This would be a ResourceLoaderWikiModule based module loading ). The callback can then be passed to

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: - ext.wikiLove.core.js</tt> would only define the $.wikiLove object and not 'do' anything yet. - ext.wikiLove.core.js</tt> would contain something like  and This wiki page may (if local wiki decides to) contain. This file would call, define  and pass   to jQuery's document ready hook.
 * Register  (jquery.elastic.js</tt>)
 * Register  (ext.wikiLove.core.js</tt>, ext.wikiLove.css</tt> and ext.wikiLove.defaultTypes.js</tt>, dependencies: jquery.elastic</tt>).
 * Register  (ResourceLoaderWikiModule: MediaWiki:WikiLove.js</tt>, dependencies: ext.wikiLove.startup</tt>)
 * Register  (ext.wikiLove.init.js</tt>, depending on ext.wikiLove.local</tt>).

Then only addModule  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  (not through  ). Some wikis override this path (such as Wikimedia). But also many users that run a wiki off trunk, that have /trunk/extensions</tt> and /trunk/phase3</tt> and alias the former to /trunk/phase3/extensions/my-symbolic-link</tt>), who set  to that. In those cases   would be a 404 error.

Interface images
I see several html elements containing a single which I think may be better done through CSS with classes and background images. This also enables ResourceLoader to convert them (when indicated with ) 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  placeholders, then the HTML string is passed to jQuery, then some additional manipulation is done, and finally   to solve the placeholders at once).

Selectors
jQuery's Sizzle engine (like the browsers native CSS engine) goes from right to left. gets all Anchor tags on the page, and the parent is checked of each to match #wlTypes. Aside from that, jQuery has a  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):

preventDefault
Returning false from a callback function 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 " " 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:
 * vs.

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

ID and Class names
The CSS IDs and classes appear a bit general to me compared to our convention of prefixing. Especially IDs like " " and " " could potentially appear in an article's heading and thus as ID on that. IDs/Classes like  or   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  to. ""
 * Chaining:
 * 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
 * Due to jQuery's built-in support for group-manipulations (it's optimized for group manipulations), the following can be shortened and speed up:
 * By using a comma seperated CSS selector and caching the jQuery object outside the functions scope:

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

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

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

As of 1.17 a utility function exists that takes care of all this:

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
 * overly specific rule

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.