r112930 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r112929‎ | r112930 | r112931 >
Date:00:50, 3 March 2012
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
[CentralAuth] Refactor JS
* Prefix variable definitions with 'var'. Previously `methodHint` was occupying the global namespace ("ack" search reveals no usage of this stuff in ./trunk/phase3 or ./trunk/extensions other than this dir)
* Remove wg prefix of the variable to avoid confusion with wiki globals
* Binding hideMethodHint directly to methodHint.onclick. No need to set an attribute and let eval()-behavior on a javascript string
* Using ResourceLoader module messages (and client side: mw.msg and mw.message), instead of exposing a global in javascript with pre-processed raw html (wfMsgWikiHtml).
* Remove redundant hideMethodHint
* Fix onclick="showMethodHint('{$method}')" by using a data attribute and binding a delegated event handler on the container and showing the method hint that way
* Hiding the (?) with CSS if javascript isn't running
* Clean up JS to use mw and jQuery, adding nice little fadeIn/Out to the hint bubble, and re-using the same hint element instead of re-creating it
* Other mis

* Fixes:
-- Bug 34915 - Uncaught ReferenceError: showMethodHint is not defined
-- Bug 34916 - Port CentralAuth to use ResourceLoader
* Pokes: r112925, r112926
Modified paths:
  • /trunk/extensions/CentralAuth/CentralAuth.php (modified) (history)
  • /trunk/extensions/CentralAuth/modules/ext.centralauth.css (modified) (history)
  • /trunk/extensions/CentralAuth/modules/ext.centralauth.js (modified) (history)
  • /trunk/extensions/CentralAuth/specials/SpecialCentralAuth.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralAuth/CentralAuth.php
@@ -260,6 +260,22 @@
261261 $wgResourceModules['ext.centralauth'] = array(
262262 'scripts' => 'ext.centralauth.js',
263263 'styles' => 'ext.centralauth.css',
 264+ 'messages' => array(
 265+ 'centralauth-merge-method-primary',
 266+ 'centralauth-merge-method-primary-desc',
 267+ 'centralauth-merge-method-new',
 268+ 'centralauth-merge-method-new-desc',
 269+ 'centralauth-merge-method-empty',
 270+ 'centralauth-merge-method-empty-desc',
 271+ 'centralauth-merge-method-password',
 272+ 'centralauth-merge-method-password-desc',
 273+ 'centralauth-merge-method-mail',
 274+ 'centralauth-merge-method-mail-desc',
 275+ 'centralauth-merge-method-admin',
 276+ 'centralauth-merge-method-admin-desc',
 277+ 'centralauth-merge-method-login',
 278+ 'centralauth-merge-method-login-desc',
 279+ ),
264280 ) + $commonModuleInfo;
265281
266282 $wgResourceModules['ext.centralauth.noflash'] = array(
Index: trunk/extensions/CentralAuth/specials/SpecialCentralAuth.php
@@ -416,11 +416,26 @@
417417 */
418418 function formatMergeMethod( $method ) {
419419 global $wgExtensionAssetsPath;
 420+
 421+ $brief = wfMessage( 'centralauth-merge-method-{$method}' )->text();
 422+ $html =
 423+ Html::element(
 424+ 'img', array(
 425+ 'src' => "{$wgExtensionAssetsPath}/CentralAuth/icons/merged-{$method}.png",
 426+ 'alt' => $brief,
 427+ 'title' => $brief,
 428+ )
 429+ )
 430+ . Html::element(
 431+ 'span', array(
 432+ 'class' => 'merge-method-help',
 433+ 'title' => $brief,
 434+ 'data-centralauth-mergemethod' => $method
 435+ ),
 436+ '(?)'
 437+ );
420438
421 - $img = htmlspecialchars( "{$wgExtensionAssetsPath}/CentralAuth/icons/merged-{$method}.png" );
422 - $brief = wfMsgHtml( "centralauth-merge-method-{$method}" );
423 - return "<img src=\"{$img}\" alt=\"{$brief}\" title=\"{$brief}\"/>" .
424 - "<span class=\"merge-method-help\" title=\"{$brief}\" onclick=\"showMethodHint('{$method}')\">(?)</span>";
 439+ return $html;
425440 }
426441
427442 /**
Index: trunk/extensions/CentralAuth/modules/ext.centralauth.css
@@ -12,3 +12,8 @@
1313 .merge-method-help-name {
1414 font-weight: bold;
1515 }
 16+
 17+/* Hide them when javascript fails or isn't enabled */
 18+.client-nojs .mw-centralauth-wikislist .merge-method-help {
 19+ display: none;
 20+}
Index: trunk/extensions/CentralAuth/modules/ext.centralauth.js
@@ -1,34 +1,57 @@
2 -wgCursorPosition = { x : 0, y : 0 };
3 -function updateCursorPosition( e ) {
4 - e = e || window.event;
5 - wgCursorPosition.x = e.clientX + ( document.documentElement.scrollLeft || document.body.scrollLeft )
6 - - document.documentElement.clientLeft;
7 - wgCursorPosition.y = e.clientY + ( document.documentElement.scrollTop || document.body.scrollTop )
8 - - document.documentElement.clientTop;
9 -}
10 -document.onmousemove = updateCursorPosition;
 2+( function ( mw, $, undefined ) {
 3+ var cursorPosition, $methodHint;
114
12 -methodHint = null;
13 -function showMethodHint( methodName ) {
14 - hideMethodHint();
 5+ cursorPosition = {
 6+ x : 0,
 7+ y : 0
 8+ };
159
16 - method = wgMergeMethodDescriptions[methodName];
17 - helpHtml = "<p class='merge-method-help-name'>" + method.short + "</p>" + method.desc;
 10+ $(document).on( 'mousemove', function updateCursorPosition( e ) {
 11+ cursorPosition.x =
 12+ e.clientX
 13+ + ( document.documentElement.scrollLeft || document.body.scrollLeft )
 14+ - document.documentElement.clientLeft;
1815
19 - methodHint = document.createElement( 'div' );
20 - methodHint.innerHTML = helpHtml;
21 - methodHint.setAttribute( 'class', 'merge-method-help-div' );
22 - methodHint.style.left = wgCursorPosition.x + 'px';
23 - methodHint.style.top = wgCursorPosition.y + 'px';
24 - methodHint.setAttribute( 'onclick', 'hideMethodHint()' );
 16+ cursorPosition.y =
 17+ e.clientY
 18+ + ( document.documentElement.scrollTop || document.body.scrollTop )
 19+ - document.documentElement.clientTop;
 20+ } );
2521
26 - var content = document.getElementById('content') || document.getElementById('mw_content') || document.body;
27 - content.appendChild( methodHint );
28 -}
 22+ function showMethodHint( methodName ) {
 23+ var content, hintHtml;
2924
30 -function hideMethodHint() {
31 - if( methodHint ) {
32 - methodHint.parentNode.removeChild( methodHint );
33 - methodHint = null;
 25+ if ( !$methodHint ) {
 26+ $methodHint = $( '<div>' )
 27+ .addClass( 'merge-method-help-div' )
 28+ .hide()
 29+ .click( function () {
 30+ $(this).fadeOut();
 31+ } );
 32+
 33+ content = document.getElementById( 'content' ) || document.getElementById( 'mw_content' ) || document.body;
 34+ $(content).append( $methodHint );
 35+ }
 36+
 37+ hintHtml = mw.html.element( 'p', {
 38+ 'class': 'merge-method-help-name'
 39+ }, mw.msg( 'centralauth-merge-method-' + methodName ) ) + mw.message( 'centralauth-merge-method-' + methodName + '-desc' ).escaped();
 40+
 41+ $methodHint
 42+ .html( hintHtml )
 43+ .css({
 44+ left: cursorPosition.x + 'px',
 45+ top: cursorPosition.y + 'px'
 46+ });
 47+
 48+ $methodHint.fadeIn();
3449 }
35 -}
 50+
 51+ $( document ).ready( function () {
 52+ // Bind an event listener to the common parent of all (?) elements
 53+ $( '.mw-centralauth-wikislist' ).on( 'click', '.merge-method-help', function () {
 54+ showMethodHint( $(this).data( 'centralauth-mergemethod' ) );
 55+ } );
 56+ } );
 57+
 58+}( mediaWiki, jQuery ) );

Sign-offs

UserFlagDate
Nikerabbitinspected14:11, 5 March 2012

Follow-up revisions

RevisionCommit summaryAuthorDate
r113884[CentralAuth JS] remove redunant function expression name, whitespace (follow...krinkle01:07, 15 March 2012
r113940MFT r112586, r112930, r113352, r113926reedy17:51, 15 March 2012
r114017MFT r112586, r112926, r112930, r113352reedy15:24, 16 March 2012
r114020Bug 35266 - tooltip isn't translated...reedy16:17, 16 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r112925[SpecialCentralAuth] Fix trailing comma in object literal. This is too much f...krinkle22:37, 2 March 2012
r112926[SpecialCentralAuth] Refactor awful hackish hand-made json-string...krinkle22:59, 2 March 2012

Comments

#Comment by Catrope (talk | contribs)   21:08, 14 March 2012
+ $(document).on( 'mousemove', function updateCursorPosition( e ) {

This seems to be legal for some reason, but why would you want to write it this way?

Status & tagging log