User "Krinkle" posted a comment on MediaWiki.r98563.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c23636
Commit summary:
Followup r98500, conversion of the Javascript to use jQuery fully. It also
makes the Javascript work outside of debug=1 (oops)
There's a couple of WTF moments in the code, this is really due to some issues
with the PHP side of stuff. It really needs some TLC but for right now it
works, I'll come back to it before 1.19 and clean it up.
Comment:
<pre>
+new ( function( $, mw ) {
</pre>
I'd recommend turning this into an object instead of an anonymous
immediately-invoked object constructor. Nothing in here has "state", they're
basically 'static' functions.
If at some point it would have state, or if you prefer to have this as a
constructor/class, then assign the constructor to a variable and instantiate it
later (ie. <code>var categoryTree = function(){ .. }; categoryTree.prototype {
showToggles: function(){ .. }, .. };</code> and instantiate it somewhere public
(i.e. <code>mw.categoryTree = new categoryTree();</code>).
Please add a little more function documentation.
<pre>
+ * @var {this}
+ */
+ var that = this;
</pre>
'this' is not a variable type. It would be {Function} or {Object}. This would
be removed if turning it into an object or a named constructor though.
<pre>
+ /**
+ * Handles clicks on the expand buttons, and calls the appropriate
function
+ */
+ this.handleNode = function() {
+ $link = $( this );
</pre>
The reference to this here is an element node, however this is not documented
anywhere, something like <code>@context {Element} CategoryTreeToggle</code>
would make it clear that 'this' is not referring to the category tree object
but to the elemtn. It also appears to not have any arguments, however when
called like this <code>.click( that.handleNode );</code> it would have a first
arguments of <code>@param e {jQuery.Event}</code>. Please document this in the
function, as the function format should be defined by the function, not by the
caller.
Please validate through JSHint.com, there are some small potential issues with
loose comparisons to 0 and empty strings (use <code>===</code> instead), and
some missing semi-colons.
<pre>+ data = $( '<i
class="CategoryTreeNotice" />' ).text( data );</pre>
This will fail in IE as it is invalid HTML. Self-closing tags on elements that
don't support this behavior cause the fragment to fail on IE6/7. Modern
browsers today auto-correct it, but that's only up to a certain point. Should
be valid HTML.
<pre>
+ $children.html(
+ '<i class="CategoryTreeNotice">'
+ + mw.msg( 'categorytree-loading' ) + "</i>"
+ );
</pre>
Potential html injection here. Should use .text() or mw.html.escape() instead
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
