r90943 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90942‎ | r90943 | r90944 >
Date:06:40, 28 June 2011
Author:hashar
Status:resolved (Comments)
Tags:
Comment:
Disable ns selector checkboxes when 'all' namespace is selected
* based on an idea by Aaron on r90866
* comes with QUnit test
* expect the special 'all' namespace to be the first in the list
* function build on mediawiki.special form r90941
Modified paths:
  • /trunk/phase3/includes/Xml.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialRecentchanges.php (modified) (history)
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/mediawiki.special/mediawiki.special.recentchanges.js (added) (history)
  • /trunk/phase3/tests/qunit/index.html (modified) (history)
  • /trunk/phase3/tests/qunit/suites/resources/mediawiki.special (added) (history)
  • /trunk/phase3/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.js (added) (history)

Diff [purge]

Index: trunk/phase3/resources/Resources.php
——@@ -534,6 +534,13 @@
535535 'scripts' => 'resources/mediawiki.special/mediawiki.special.movePage.js',
536536 'dependencies' => 'jquery.byteLimit',
537537 ),
 538+ 'mediawiki.special' => array(
 539+ 'scripts' => 'resources/mediawiki/mediawiki.special.js',
 540+ ),
 541+ 'mediawiki.special.recentchanges' => array(
 542+ 'scripts' => 'resources/mediawiki.special/mediawiki.special.recentchanges.js',
 543+ 'dependencies' => array( 'mediawiki.special' ),
 544+ ),
538545 'mediawiki.special.upload' => array(
539546 // @TODO: merge in remainder of mediawiki.legacy.upload
540547 'scripts' => 'resources/mediawiki.special/mediawiki.special.upload.js',
Index: trunk/phase3/resources/mediawiki.special/mediawiki.special.recentchanges.js
——@@ -0,0 +1,38 @@
 2+/* JavaScript for Special:RecentChanges */
 3+( function( $, mw ) {
 4+
 5+mw.special.recentchanges = {
 6+ // -- Variables
 7+ 'select' : false,
 8+ 'checkboxes' : [ 'nsassociated', 'nsinvert' ],
 9+
 10+ // -- Methods
 11+ 'init' : function() {
 12+ this.select = $( 'select#namespace' );
 13+
 14+ // Register an onChange trigger for the <select> element
 15+ this.select.change( function() {
 16+ mw.special.recentchanges.updateCheckboxes();
 17+ });
 18+ // on load, trigger the event to eventually update checkboxes statuses
 19+ this.select.change();
 20+ },
 21+
 22+ /**
 23+ * handler to disable/enable the namespace selector checkboxes when the
 24+ * special 'all' namespace is selected/unselected respectively.
 25+ */
 26+ 'updateCheckboxes' : function() {
 27+ // The 'all' namespace is the FIRST in the list.
 28+ var isAllNS = this.select.find( 'option' ).first().is( ':selected' );
 29+
 30+ // Iterates over checkboxes and propagate the selected option
 31+ $.map( this.checkboxes, function(id) {
 32+ $( 'input#'+id ).attr( 'disabled', isAllNS );
 33+ });
 34+ },
 35+};
 36+
 37+mw.special.recentchanges.init();
 38+
 39+}(jQuery, mediaWiki ) );
Property changes on: trunk/phase3/resources/mediawiki.special/mediawiki.special.recentchanges.js
___________________________________________________________________
Added: svn:eol-style
140 + native
Index: trunk/phase3/tests/qunit/index.html
——@@ -44,6 +44,8 @@
4545 <script src="../../resources/jquery/jquery.tabIndex.js"></script>
4646 <script src="../../resources/jquery/jquery.tablesorter.js"></script>
4747 <script src="../../resources/mediawiki/mediawiki.Title.js"></script>
 48+ <script src="../../resources/mediawiki/mediawiki.special.js"></script>
 49+ <script src="../../resources/mediawiki.special/mediawiki.special.recentchanges.js"></script>
4850
4951 <!-- QUnit: Load framework -->
5052 <link rel="stylesheet" href="../../resources/jquery/jquery.qunit.css" />
——@@ -62,6 +64,7 @@
6365 <script src="suites/resources/jquery/jquery.tabIndex.js"></script>
6466 <script src="suites/resources/jquery/jquery.tablesorter.test.js" charset="UTF-8"></script>
6567 <script src="suites/resources/mediawiki/mediawiki.Title.js"></script>
 68+ <script src="suites/resources/mediawiki.special/mediawiki.special.recentchanges.js"></script>
6669
6770 <!-- TestSwarm: If a test swarm is running this,
6871 the following script will allow it to extract the results.
Index: trunk/phase3/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.js
——@@ -0,0 +1,67 @@
 2+module( 'mediawiki.special.preferences.js' );
 3+
 4+test( '-- Initial check', function() {
 5+ expect( 2 );
 6+ ok( mediaWiki.special.recentchanges.init,
 7+ 'mediaWiki.special.recentchanges.init defined'
 8+ );
 9+ ok( mediaWiki.special.recentchanges.updateCheckboxes,
 10+ 'mediaWiki.special.recentchanges.updateCheckboxes defined'
 11+ );
 12+ // TODO: verify checkboxes == [ 'nsassociated', 'nsinvert' ]
 13+});
 14+
 15+test( 'foobar', function() {
 16+
 17+ // from Special:Recentchanges
 18+ var select =
 19+ '<select id="namespace" name="namespace" class="namespaceselector">'
 20+ + '<option value="" selected="selected">all</option>'
 21+ + '<option value="0">(Main)</option>'
 22+ + '<option value="1">Talk</option>'
 23+ + '<option value="2">User</option>'
 24+ + '<option value="3">User talk</option>'
 25+ + '<option value="4">ProjectName</option>'
 26+ + '<option value="5">ProjectName talk</option>'
 27+ + '</select>'
 28+ + '<input name="invert" type="checkbox" value="1" id="nsinvert" title="no title" />'
 29+ + '<label for="nsinvert" title="no title">Invert selection</label>'
 30+ + '<input name="associated" type="checkbox" value="1" id="nsassociated" title="no title" />'
 31+ + '<label for="nsassociated" title="no title">Associated namespace</label>'
 32+ + '<input type="submit" value="Go" />'
 33+ + '<input type="hidden" value="Special:RecentChanges" name="title" />'
 34+ ;
 35+
 36+ var $env = $( '<div>' ).html( select ).appendTo( 'body' );
 37+ var enabled = undefined;
 38+
 39+ // TODO abstract the double strictEquals
 40+
 41+ // At first checkboxes are enabled
 42+ strictEqual( $('input#nsinvert').attr('disabled'), enabled);
 43+ strictEqual( $('input#nsassociated').attr('disabled'), enabled);
 44+
 45+ // load our magic code to disable them
 46+ mediaWiki.special.recentchanges.init();
 47+ strictEqual( $('#nsinvert').attr('disabled'), 'disabled');
 48+ strictEqual( $('#nsassociated').attr('disabled'), 'disabled' );
 49+
 50+ // select second option...
 51+ $('select#namespace option:nth-child(1)').removeAttr( 'selected' );
 52+ $('select#namespace option:nth-child(2)').attr( 'selected', 'selected' );
 53+ $('select#namespace').change();
 54+ // ... and checkboxes should be enabled again
 55+ strictEqual( $('input#nsinvert').attr('disabled'), enabled);
 56+ strictEqual( $('input#nsassociated').attr('disabled'), enabled);
 57+
 58+ // select first option ('all' namespace)...
 59+ $('select#namespace option:nth-child(1)').attr( 'selected', 'selected' );
 60+ $('select#namespace option:nth-child(2)').removeAttr( 'selected' );
 61+ $('select#namespace').change();
 62+ // ... and checkboxes should now be disabled
 63+ strictEqual( $('#nsinvert').attr('disabled'), 'disabled');
 64+ strictEqual( $('#nsassociated').attr('disabled'), 'disabled' );
 65+
 66+ // DOM cleanup
 67+ $env.remove();
 68+});
Property changes on: trunk/phase3/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.js
___________________________________________________________________
Added: svn:eol-style
169 + native
Index: trunk/phase3/includes/Xml.php
——@@ -132,6 +132,8 @@
133133 }
134134
135135 if( !is_null( $all ) )
 136+ # Please make sure the 'namespacesall' is the first or you will break
 137+ # such an assumption (ex js: mw.special.recentchanges.updateCheckboxes)
136138 $namespaces = array( $all => wfMsg( 'namespacesall' ) ) + $namespaces;
137139 foreach( $namespaces as $index => $name ) {
138140 if( $index < NS_MAIN ) {
Index: trunk/phase3/includes/specials/SpecialRecentchanges.php
——@@ -140,6 +140,7 @@
141141 $opts = $this->getOptions();
142142 $this->setHeaders();
143143 $this->outputHeader();
 144+ $this->addRecentChangesJS();
144145
145146 // Fetch results, prepare a batch link existence check query
146147 $conds = $this->buildMainQueryConds( $opts );
——@@ -839,4 +840,14 @@
840841 $rclistfrom = wfMsgExt( 'rclistfrom', array( 'parseinline', 'replaceafter' ), $tl );
841842 return "{$note}$rclinks<br />$rclistfrom";
842843 }
 844+
 845+ /**
 846+ * add javascript specific to the [[Special:RecentChanges]] page
 847+ */
 848+ function addRecentChangesJS() {
 849+ global $wgOut;
 850+ $wgOut->addModules( array(
 851+ 'mediawiki.special.recentchanges',
 852+ ) );
 853+ }
843854 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r90960Fix r90943: redefined mediawiki.special module that had already been added in...catrope14:28, 28 June 2011
r90982mw.special.recentchanges fixes:...krinkle17:40, 28 June 2011
r90984mw.special.recentchanges fixes:...krinkle17:56, 28 June 2011
r90990Find 'all' special ns regardless of its rank in the select...hashar19:32, 28 June 2011
r90991rename test for the ns selector checkboxes...hashar19:35, 28 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r90866Tweaked r90766 messages. Still awkward. Also, this still needs JS to disable ...aaron23:50, 26 June 2011
r90941mediawiki.special as a dumb JS object...hashar05:57, 28 June 2011

Comments

#Comment by Krinkle (talk | contribs)   17:04, 28 June 2011 
+module( 'mediawiki.special.preferences.js' );

There is no test suite by that name. Causes black holes on the TestSwarm. 


+( function( $, mw ) {

+}(jQuery, mediaWiki ) );

+	ok( mediaWiki.special.recentchanges.init,

mw is a global alias, you can use it directly. 


+		this.select = $( 'select#namespace' );
+			$( 'input#'+id ).attr( 'disabled', isAllNS );
+	strictEqual( $('input#nsinvert').attr('disabled'), enabled);

IDs should be unique, if they aren't then only one will match the ID selector. Passing a tagname doens't help other than slowing down the selector.

#Comment by Brion VIBBER (talk | contribs)   17:48, 28 June 2011

mw is a global alias, you can use it directly.

Aliasing global variables to a local variable is a common JS pattern; it can allow the JS engine to better optimize lookup (don't have to bump back multiple levels of scoping context every time it's referenced; can potentially be better assured that the value won't change depending on how much of the code the optimizer sees at once).
#Comment by Brion VIBBER (talk | contribs)   17:56, 28 June 2011

Test issues seem to have been cleaned up, and Krinkle made some style tweaks as well.

It still though assumes that 'all' is the first item -- shouldn't it just check the value of the selected option? means none/all, whereas a number means some particular namespace.

#Comment by Brion VIBBER (talk | contribs)   17:56, 28 June 2011

oh wikitext. :) make that ""

#Comment by Hashar (talk | contribs)   19:33, 28 June 2011

Yes wikitext. That is why I assumed it as the first element. r90990 makes it look per value as you suggested.

#Comment by Brion VIBBER (talk | contribs)   22:03, 28 June 2011

Ok, all looks good. marking resolved. :D

#Comment by Hashar (talk | contribs)   05:35, 29 June 2011

Thank you both for the fast code review :-)

Status & tagging log