r98096 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r98095‎ | r98096 | r98097 >
Date:23:16, 25 September 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
[flaggedrevs.js]: Code cleanup
* Apply whitespace conventions
* Apply best practices and other code conventions (strict comparison, combined var statements, local function closure)
* Alias jQuery to $ locally
* Use $.fn.prop() instead of $.fn.attr() to access the 'checked' property. $.fn.attr() still supports this for backwards compatibility (it reroutes to $.fn.prop if keys like 'checked' are given), but it should not be used in new code. 'title' is an attribute however.
* Add function documentation where missing. Making it obvious that the context ("this") of the functions called from the mouseout/click handlers is the jQuery object. Also emphasizing that the 'e' parameter passed is an instance of jQuery.Event, which is a normalized version of the native object thrown by the browser.
* Remove cross-browser compatibility check of e.relatedTarget as jQuery.Event normalizes this and sets e.relatedTarget to e.toElement in IE.
* Remove bogus comment "//event bubbling" in isMouseOutBubble. Although it is called from an event handler, it is not the event handler itself. Returning true or false from isMouseOutBubble has no influence on the event handler. The function is called from an if-statement in onBoxMouseOut().
* Make use of prototype chaining instead of re-referencing the jQuery object where possible (ie. "save.val( .. ); save.attr( .. );" -> "save.val( .. ).attr( .. );")
* Make a local variable definition and expose it globally later instead of declaring it into window and referring to it without "window." which makes refers to an implied global instead of to the global directly. Another way to solve that is to use "window.FlaggedRevs" everywhere, but choose to use a local variable instead to make it shorter.
--
(This revision follows-up r98078)
Modified paths:
  • /trunk/extensions/FlaggedRevs/presentation/modules/flaggedrevs.js (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/presentation/modules/flaggedrevs.js
@@ -3,164 +3,210 @@
44 * @author Aaron Schulz
55 * @author Krinkle <krinklemail@gmail.com> 2011
66 */
 7+( function( $ ) {
78
8 -window.FlaggedRevs = {
 9+var fr = {
910 /* Dropdown collapse timer */
1011 'boxCollapseTimer': null,
1112
1213 /* Startup function */
1314 'init': function() {
1415 // Enables rating detail box
15 - var toggle = $('#mw-fr-revisiontoggle');
 16+ var toggle = $( '#mw-fr-revisiontoggle' );
 17+
1618 if ( toggle.length ) {
17 - toggle.css('display','inline'); /* show toggle control */
18 - FlaggedRevs.hideBoxDetails(); /* hide the initially displayed ratings */
 19+ toggle.css( 'display', 'inline' ); // show toggle control
 20+ fr.hideBoxDetails(); // hide the initially displayed ratings
1921 }
 22+
2023 // Bar UI: Toggle the box when the toggle is clicked
21 - $('.fr-toggle-symbol#mw-fr-revisiontoggle').click( FlaggedRevs.toggleBoxDetails );
 24+ $( '.fr-toggle-symbol#mw-fr-revisiontoggle' ).click( fr.toggleBoxDetails );
 25+
2226 // Simple UI: Show the box on mouseOver
23 - $('.fr-toggle-arrow#mw-fr-revisiontoggle').mouseover( FlaggedRevs.onBoxMouseOver );
24 - $('.flaggedrevs_short#mw-fr-revisiontag').mouseout( FlaggedRevs.onBoxMouseOut );
25 -
 27+ $( '.fr-toggle-arrow#mw-fr-revisiontoggle' ).mouseover( fr.onBoxMouseOver );
 28+ $( '.flaggedrevs_short#mw-fr-revisiontag' ).mouseout( fr.onBoxMouseOut );
 29+
2630 // Enables diff detail box and toggle
27 - toggle = $('#mw-fr-difftoggle');
 31+ toggle = $( '#mw-fr-difftoggle' );
2832 if ( toggle.length ) {
29 - toggle.css('display','inline'); /* show toggle control */
30 - $('#mw-fr-stablediff').hide();
 33+ toggle.css( 'display', 'inline' ); // show toggle control
 34+ $( '#mw-fr-stablediff' ).hide();
3135 }
32 - toggle.children('a').click( FlaggedRevs.toggleDiff );
33 -
 36+ toggle.children( 'a' ).click( fr.toggleDiff );
 37+
3438 // Enables log detail box and toggle
35 - toggle = $('#mw-fr-logtoggle');
 39+ toggle = $( '#mw-fr-logtoggle' );
3640 if ( toggle.length ) {
37 - toggle.css('display','inline'); /* show toggle control */
38 - $('#mw-fr-logexcerpt').hide();
 41+ toggle.css( 'display', 'inline' ); // show toggle control
 42+ $( '#mw-fr-logexcerpt' ).hide();
3943 }
40 - toggle.children('a').click( FlaggedRevs.toggleLog );
41 -
 44+ toggle.children( 'a' ).click( fr.toggleLog );
 45+
4246 // Enables changing of save button when "review this" checkbox changes
43 - $('#wpReviewEdit').click( FlaggedRevs.updateSaveButton );
 47+ $( '#wpReviewEdit' ).click( fr.updateSaveButton );
4448 },
4549
4650 /* Expands flag info box details */
4751 'showBoxDetails': function() {
48 - $('#mw-fr-revisiondetails').css('display','block');
 52+ $( '#mw-fr-revisiondetails' ).css( 'display', 'block' );
4953 },
5054
5155 /* Collapses flag info box details */
52 - 'hideBoxDetails': function( event ) {
53 - $('#mw-fr-revisiondetails').css('display','none');
 56+ 'hideBoxDetails': function() {
 57+ $( '#mw-fr-revisiondetails' ).css( 'display', 'none' );
5458 },
5559
56 - /* Toggles flag info box details for (+/-) control */
57 - 'toggleBoxDetails': function() {
58 - var toggle = $('#mw-fr-revisiontoggle');
59 - var ratings = $('#mw-fr-revisiondetails');
 60+ /**
 61+ * Toggles flag info box details for (+/-) control
 62+ * @context {jQuery}
 63+ * @param e {jQuery.Event}
 64+ */
 65+ 'toggleBoxDetails': function( e ) {
 66+ var toggle = $( '#mw-fr-revisiontoggle' ),
 67+ ratings = $( '#mw-fr-revisiondetails' );
 68+
6069 if ( toggle.length && ratings.length ) {
6170 // Collapsed -> expand
62 - if ( ratings.css('display') == 'none' ) {
63 - FlaggedRevs.showBoxDetails();
64 - toggle.text( mw.msg('revreview-toggle-hide') );
 71+ if ( ratings.css( 'display' ) === 'none' ) {
 72+ fr.showBoxDetails();
 73+ toggle.text( mw.msg( 'revreview-toggle-hide' ) );
6574 // Expanded -> collapse
6675 } else {
67 - FlaggedRevs.hideBoxDetails();
68 - toggle.text( mw.msg('revreview-toggle-show') );
 76+ fr.hideBoxDetails();
 77+ toggle.text( mw.msg( 'revreview-toggle-show' ) );
6978 }
7079 }
7180 },
7281
73 - /* Expands flag info box details on mouseOver */
74 - 'onBoxMouseOver': function( event ) {
75 - window.clearTimeout( FlaggedRevs.boxCollapseTimer );
76 - FlaggedRevs.boxCollapseTimer = null;
77 - FlaggedRevs.showBoxDetails();
 82+ /**
 83+ * Expands flag info box details on mouseOver
 84+ * @context {jQuery}
 85+ * @param e {jQuery.Event}
 86+ */
 87+ 'onBoxMouseOver': function( e ) {
 88+ window.clearTimeout( fr.boxCollapseTimer );
 89+ fr.boxCollapseTimer = null;
 90+ fr.showBoxDetails();
7891 },
7992
80 - /* Hides flag info box details on mouseOut *except* for event bubbling */
81 - 'onBoxMouseOut': function( event ) {
82 - if ( !FlaggedRevs.isMouseOutBubble( event, 'mw-fr-revisiontag' ) ) {
83 - FlaggedRevs.boxCollapseTimer = window.setTimeout( FlaggedRevs.hideBoxDetails, 150 );
 93+ /**
 94+ * Hides flag info box details on mouseOut *except* for event bubbling
 95+ * @context {jQuery}
 96+ * @param e {jQuery.Event}
 97+ */
 98+ 'onBoxMouseOut': function( e ) {
 99+ if ( !fr.isMouseOutBubble( e, 'mw-fr-revisiontag' ) ) {
 100+ fr.boxCollapseTimer = window.setTimeout( fr.hideBoxDetails, 150 );
84101 }
85102 },
86103
87 - /* Checks if mouseOut event is for a child of parentId */
88 - 'isMouseOutBubble': function( event, parentId ) {
89 - var toNode = null;
90 - if ( event.relatedTarget !== undefined ) {
91 - toNode = event.relatedTarget; // FF/Opera/Safari
92 - } else {
93 - toNode = event.toElement; // IE
94 - }
 104+ /**
 105+ * Checks if mouseOut event is for a child of parentId
 106+ * @param e {jQuery.Event}
 107+ * @param parentId {String}
 108+ * @return {Boolean} True if given event object originated from a (direct or indirect)
 109+ * child element of an element with an id of parentId.
 110+ */
 111+ 'isMouseOutBubble': function( e, parentId ) {
 112+ var toNode = e.relatedTarget;
 113+
95114 if ( toNode ) {
96115 var nextParent = toNode.parentNode;
97116 while ( nextParent ) {
98 - if ( nextParent.id == parentId ) {
99 - return true; // event bubbling
 117+ if ( nextParent.id === parentId ) {
 118+ return true;
100119 }
101 - nextParent = nextParent.parentNode; // next up
 120+ // next up
 121+ nextParent = nextParent.parentNode;
102122 }
103123 }
104124 return false;
105125 },
106126
107 - /* Toggles diffs */
108 - 'toggleDiff': function() {
109 - var diff = $('#mw-fr-stablediff');
110 - var toggle = $('#mw-fr-difftoggle');
 127+ /**
 128+ * Toggles diffs
 129+ * @context {jQuery}
 130+ * @param e {jQuery.Event}
 131+ */
 132+ 'toggleDiff': function( e ) {
 133+ var diff = $( '#mw-fr-stablediff' ),
 134+ toggle = $( '#mw-fr-difftoggle' );
 135+
111136 if ( diff.length && toggle.length ) {
112 - if ( diff.css('display') == 'none' ) {
 137+ if ( diff.css( 'display' ) === 'none' ) {
113138 diff.show( 'slow' );
114 - toggle.children('a').text( mw.msg('revreview-diff-toggle-hide') );
 139+ toggle.children( 'a' ).text( mw.msg( 'revreview-diff-toggle-hide' ) );
115140 } else {
116141 diff.hide( 'slow' );
117 - toggle.children('a').text( mw.msg('revreview-diff-toggle-show') );
 142+ toggle.children( 'a' ).text( mw.msg( 'revreview-diff-toggle-show' ) );
118143 }
119144 }
120145 },
121146
122 - /* Toggles log excerpts */
123 - 'toggleLog': function() {
124 - var log = $('#mw-fr-logexcerpt');
125 - var toggle = $('#mw-fr-logtoggle');
 147+ /**
 148+ * Toggles log excerpts
 149+ * @context {jQuery}
 150+ * @param e {jQuery.Event}
 151+ */
 152+ 'toggleLog': function( e ) {
 153+ var hideMsg, showMsg,
 154+ log = $( '#mw-fr-logexcerpt' ),
 155+ toggle = $( '#mw-fr-logtoggle' );
 156+
126157 if ( log.length && toggle.length ) {
127158 // Two different message sets used here...
128 - if ( toggle.hasClass('fr-logtoggle-details') ) {
129 - var hideMsg = mw.msg('revreview-log-details-hide');
130 - var showMsg = mw.msg('revreview-log-details-show');
 159+ if ( toggle.hasClass( 'fr-logtoggle-details' ) ) {
 160+ hideMsg = mw.msg( 'revreview-log-details-hide' );
 161+ showMsg = mw.msg( 'revreview-log-details-show' );
131162 } else {
132 - var hideMsg = mw.msg('revreview-log-toggle-hide');
133 - var showMsg = mw.msg('revreview-log-toggle-show');
 163+ hideMsg = mw.msg( 'revreview-log-toggle-hide' );
 164+ showMsg = mw.msg( 'revreview-log-toggle-show' );
134165 }
135 - if ( log.css('display') == 'none' ) {
 166+
 167+ if ( log.css( 'display' ) === 'none' ) {
136168 log.show();
137 - toggle.children('a').text( hideMsg );
 169+ toggle.children( 'a' ).text( hideMsg );
138170 } else {
139171 log.hide();
140 - toggle.children('a').text( showMsg );
 172+ toggle.children( 'a' ).text( showMsg );
141173 }
142174 }
143175 },
144176
145 - /* Update save button when "review this" checkbox changes */
 177+ /**
 178+ * Update save button when "review this" checkbox changes
 179+ * @context {jQuery}
 180+ * @param e {jQuery.Event}
 181+ */
146182 'updateSaveButton': function() {
147 - var save = $('#wpSave');
148 - var checkbox = $('#wpReviewEdit');
149 - if ( save.length && checkbox.length ) {
 183+ var $save = $( '#wpSave' ),
 184+ $checkbox = $( '#wpReviewEdit' );
 185+
 186+ if ( $save.length && $checkbox.length ) {
150187 // Review pending changes
151 - if ( checkbox.attr('checked') ) {
152 - save.val( mw.msg('savearticle') );
153 - save.attr( 'title',
154 - mw.msg('tooltip-save') + ' [' + mw.msg('accesskey-save') + ']' );
 188+ if ( $checkbox.prop( 'checked' ) ) {
 189+ $save
 190+ .val( mw.msg( 'savearticle' ) )
 191+ .attr( 'title',
 192+ mw.msg( 'tooltip-save' ) + ' [' + mw.msg( 'accesskey-save' ) + ']'
 193+ );
155194 // Submit for review
156195 } else {
157 - save.val( mw.msg('revreview-submitedit') );
158 - save.attr( 'title',
159 - mw.msg('revreview-submitedit-title') + ' [' + mw.msg('accesskey-save') + ']' );
 196+ $save
 197+ .val( mw.msg( 'revreview-submitedit' ) )
 198+ .attr( 'title',
 199+ mw.msg( 'revreview-submitedit-title' ) + ' [' + mw.msg( 'accesskey-save' ) + ']'
 200+ );
160201 }
161202 }
162 - mw.util.updateTooltipAccessKeys( [ save ] ); // update accesskey in save.title
 203+ mw.util.updateTooltipAccessKeys( [ $save ] ); // update accesskey in save.title
163204 }
164205 };
165206
166207 // Perform some onload (which is when this script is included) events:
167 -FlaggedRevs.init();
 208+fr.init();
 209+
 210+// Expose globally
 211+window.FlaggedRevs = fr;
 212+
 213+})( jQuery );
\ No newline at end of file

Follow-up revisions

RevisionCommit summaryAuthorDate
r98099r98096: Add new line at EoFkrinkle00:34, 26 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r98078* Made flaggedrevs.js use jquery...aaron19:22, 25 September 2011
r98082* Removed references to FlaggedRevs JS object in PHP. Set event handlers all ...aaron20:28, 25 September 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   23:29, 25 September 2011

Hmm, I noticed there is no newline at the EoF.

Feel like doing review.js? :)

#Comment by Krinkle (talk | contribs)   00:34, 26 September 2011

Perhaps some other time, since for that I feel like I should actually install the extension properly, which I don't have at the moment.

Status & tagging log