[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added, and revision status changed
"Brion VIBBER" changed the status of MediaWiki.r98563 to "ok" and commented it. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c30957 Old Status: new > New Status: ok Commit summary for MediaWiki.r98563: 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. Brion VIBBER's comment: Issues appear to have been resolved. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
"Brion VIBBER" posted a comment on MediaWiki.r98563. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c30956 Commit summary for MediaWiki.r98563: 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. Brion VIBBER's comment: was resolved r111218 ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
"Reedy" posted a comment on MediaWiki.r98563. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c30108 Commit summary for MediaWiki.r98563: 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. Reedy's comment: See bug 33989, might be this revision at fault ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
"Catrope" posted a comment on MediaWiki.r98563. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c29195 Commit summary for MediaWiki.r98563: 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. Catrope's comment: Did you ever "come back before 1.19 and clean it up" like you promised in the commit message? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
"Nikerabbit" posted a comment on MediaWiki.r98563. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c28974 Commit summary for MediaWiki.r98563: 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. Nikerabbit's comment: Don't you want to use .text here and elsewhere: + .html( mw.msg( 'categorytree-collapse-bullet' ) ) ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added, and revision status changed
User "OverlordQ" changed the status of MediaWiki.r98563. Old Status: fixme New Status: new User "OverlordQ" also posted a comment on MediaWiki.r98563. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c24691 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: well, i had a brainfart moment, the tests are actually in Cite for some reason. Just saw it was CatTree that changed. http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/Cite/citeCatTreeParserTests.txt still should probably be updated by somebody who knows what the proper output should be. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
User "Johnduhart" posted a comment on MediaWiki.r98563. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c24665 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: What parser tests? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added, and revision status changed
User "OverlordQ" changed the status of MediaWiki.r98563. Old Status: new New Status: fixme User "OverlordQ" also posted a comment on MediaWiki.r98563. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c24660 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: Changed the output but didn't update the parser tests ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
User "Johnduhart" posted a comment on MediaWiki.r98563. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c24326 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: Was fixed in r98702 ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
User "Krinkle" posted a comment on MediaWiki.r98563.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c23654
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:
Yes. There is a raw message being inserted between other html. Either use
$('').text( mw.msg() ); Or create a
message object with mw.message( 'key' ) and call mw.message(
'key' ).escaped(). Both are useful depending on what the surrounding
code looks like.
___
MediaWiki-CodeReview mailing list
mediawiki-coderev...@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
User "Krinkle" posted a comment on MediaWiki.r98563.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c23653
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:
Yes. There is a raw message being inserted between other html. Either use
$('').text( mw.msg() ); Or create a message object with
mw.message( 'key' ) and call mw.message( 'key'
).escaped(). Both are useful depending on what the surrounding code
looks like.
___
MediaWiki-CodeReview mailing list
mediawiki-coderev...@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
User "Johnduhart" posted a comment on MediaWiki.r98563. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c23643 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: Thank you very much for the feedback, I'm constantly looking to improve my JavaScript. Potential html injection here. Should use .text() or mw.html.escape() instead Are you referring the mw.msg() call? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
User "Krinkle" posted a comment on MediaWiki.r98563.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98563#c23637
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:
new ( function( $, mw ) {
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. var categoryTree = function(){ .. }; categoryTree.prototype {
showToggles: function(){ .. }, .. }; and instantiate it somewhere public
(i.e. mw.categoryTree = new categoryTree();).
But since it doesn't have state, it's probably easiest to just but it into a
static object var categoryTree = { showToggles: function(){ .. }, ..
};.
___
MediaWiki-CodeReview mailing list
mediawiki-coderev...@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r98563]: New comment added
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:
+new ( function( $, mw ) {
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. var categoryTree = function(){ .. }; categoryTree.prototype {
showToggles: function(){ .. }, .. }; and instantiate it somewhere public
(i.e. mw.categoryTree = new categoryTree();).
Please add a little more function documentation.
+* @var {this}
+*/
+ var that = this;
'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.
+ /**
+* Handles clicks on the expand buttons, and calls the appropriate
function
+*/
+ this.handleNode = function() {
+ $link = $( this );
The reference to this here is an element node, however this is not documented
anywhere, something like @context {Element} CategoryTreeToggle
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 .click( that.handleNode ); it would have a first
arguments of @param e {jQuery.Event}. 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 === instead), and
some missing semi-colons.
+ data = $( '' ).text( data );
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.
+ $children.html(
+ ''
+ + mw.msg( 'categorytree-loading' ) + ""
+ );
Potential html injection here. Should use .text() or mw.html.escape() instead
___
MediaWiki-CodeReview mailing list
mediawiki-coderev...@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
