r96142 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r96141‎ | r96142 | r96143 >
Date:19:31, 2 September 2011
Author:junaidpv
Status:ok (Comments)
Tags:todo 
Comment:
Change position of Narayam controls to personal tools/menu area as proposed by Trevor Pascal, codes with help from Santhosh. Tested in FF, Chrome and Opera.
Modified paths:
  • /trunk/extensions/Narayam/Narayam.i18n.php (modified) (history)
  • /trunk/extensions/Narayam/Narayam.php (modified) (history)
  • /trunk/extensions/Narayam/css/ext.narayam.core.css (modified) (history)
  • /trunk/extensions/Narayam/js/ext.narayam.core.js (modified) (history)

Diff [purge]

Index: trunk/extensions/Narayam/css/ext.narayam.core.css
@@ -4,10 +4,135 @@
55 * about styling this nicely
66 */
77 background-color: #EAF3F8 !important;
8 - border: 2px inset #FDBBBB !important;
98 }
109
1110 .narayam-wrapper {
1211 padding: 0;
1312 margin: 0;
1413 }
 14+
 15+
 16+li#pt-narayam{
 17+ padding-left: 15px !important;
 18+}
 19+
 20+/* Variants and Actions */
 21+/* @noflip */
 22+div#narayam-menu {
 23+ direction: ltr;
 24+ float: left;
 25+ cursor: pointer;
 26+}
 27+div.narayam-menuFocus {
 28+ background-position: -22px 60%;
 29+}
 30+/* @noflip */
 31+body.rtl div#narayam-menu {
 32+ direction: rtl;
 33+}
 34+div#narayam-menu div.menu-items {
 35+ position: relative;
 36+ display: none;
 37+ clear: both;
 38+ text-align: left;
 39+ z-index: 99999;
 40+}
 41+
 42+div#narayam-menu-items li{
 43+ margin: 0;
 44+ padding: 5px;
 45+ font-size: 100%;
 46+ float: none;
 47+ z-index: 99999;
 48+
 49+}
 50+ul#imelist{
 51+ padding: 5px;
 52+}
 53+/* OVERRIDDEN BY COMPLIANT BROWSERS */
 54+/* @noflip */
 55+body.rtl div#narayam-menu div.menu-items {
 56+ margin-left: 24px;
 57+}
 58+/* IGNORED BY IE6 */
 59+/* @noflip */
 60+body.rtl div#narayam-menu > div.menu-items {
 61+ margin-left: auto;
 62+}
 63+/* IGNORED BY IE6 */
 64+/* Also fixes old versions of FireFox */
 65+/* @noflip */
 66+body.rtl div#narayam-menu > div.menu-items,
 67+x:-moz-any-link {
 68+ margin-left: 23px;
 69+}
 70+/* Enable forcing showing of the menu for accessibility */
 71+div#narayam-menu:hover div.menu-items, div#narayam-menu div.menuForceShow {
 72+ display: block;
 73+}
 74+div#narayam-menu ul {
 75+ position: absolute;
 76+ background-color: white;
 77+ border: solid 1px silver;
 78+ border-top-width: 0;
 79+ list-style: none;
 80+ list-style-image: none;
 81+ list-style-type: none;
 82+ padding: 0;
 83+ margin: 0;
 84+ margin-left: -1px;
 85+ text-align: left;
 86+}
 87+/* Fixes old versions of FireFox */
 88+div#narayam-menu ul,
 89+x:-moz-any-link {
 90+ min-width: 5em;
 91+}
 92+/* Returns things back to normal in modern versions of FireFox */
 93+div#narayam-menu ul,
 94+x:-moz-any-link,
 95+x:default {
 96+ min-width: 0;
 97+}
 98+div#narayam-menu li {
 99+/* padding: 0;*/
 100+ margin: 0;
 101+ text-align: left;
 102+ line-height: 1em;
 103+}
 104+/* OVERRIDDEN BY COMPLIANT BROWSERS */
 105+div#narayam-menu li a {
 106+ display: inline-block;
 107+ padding: 0.5em;
 108+ white-space: nowrap;
 109+ color: #0645ad;
 110+ cursor: pointer;
 111+/* font-size: 0.8em;*/
 112+}
 113+/* IGNORED BY IE6 */
 114+div#narayam-menu li > a {
 115+ display: block;
 116+}
 117+div#narayam-menu li.selected a,
 118+div#narayam-menu li.selected a:visited {
 119+ color: #333333;
 120+ text-decoration: none;
 121+}
 122+div#narayam-menu a {
 123+ display: block;
 124+ padding-left:5px;
 125+ padding-right:5px;
 126+}
 127+
 128+div#narayam-menu-items li.narayam-help-link a {
 129+ background: url('../images/help.png') no-repeat scroll left center transparent;
 130+ padding-left: 15px;
 131+}
 132+
 133+li.narayam-active {
 134+ background: url('../images/narayam-active.png') no-repeat scroll left top transparent;
 135+}
 136+
 137+li.narayam-inactive {
 138+ background: url('../images/narayam-inactive.png') no-repeat scroll left top transparent;
 139+}
\ No newline at end of file
Index: trunk/extensions/Narayam/Narayam.i18n.php
@@ -15,7 +15,10 @@
1616 */
1717 $messages['en'] = array(
1818 'narayam-desc' => 'Allows to add custom input methods for input fields',
19 - 'narayam-toggle-ime' => 'To toggle IM ($1)', // FIXME: better message
 19+ 'narayam-toggle-ime' => 'Enable ($1)',
 20+ 'narayam-menu' => 'Input Method',
 21+ 'narayam-menu-tooltip' => 'Control Narayam Input Method Editor (IME)',
 22+ 'narayam-help' => 'Help',
2023 'narayam-help-page' => 'Help:Typing',
2124 'narayam-checkbox-tooltip' => 'To toggle input method on and off', // FIXME: better message
2225 'narayam-disable-preference' => 'Disable Narayam Input Method Editor (IME)',
Index: trunk/extensions/Narayam/js/ext.narayam.core.js
@@ -16,13 +16,11 @@
1717 ( function( $ ) {
1818 $.narayam = new ( function() {
1919 /* Private members */
20 -
 20+
2121 // Reference to this object
2222 var that = this;
2323 // jQuery array holding all text inputs Narayam applies to
2424 var $inputs = $( [] );
25 - // Input method dropdown
26 - var $select = $( [] );
2725 // Whether Narayam is enabled
2826 var enabled = false;
2927 // Registered schemes
@@ -41,9 +39,9 @@
4240 shiftKey: false,
4341 key: null
4442 };
45 -
 43+
4644 /* Private functions */
47 -
 45+
4846 /**
4947 * Transliterate a string using the current scheme
5048 * @param str String to transliterate
@@ -68,7 +66,7 @@
6967 // No matches, return the input
7068 return str;
7169 }
72 -
 70+
7371 /**
7472 * Get the n characters in str that immediately precede pos
7573 * Example: lastNChars( "foobarbaz", 5, 2 ) == "ba"
@@ -87,7 +85,7 @@
8886 return str.substr( pos - n, n);
8987 }
9088 }
91 -
 89+
9290 /**
9391 * Find the point at which a and b diverge, i.e. the first position
9492 * at which they don't have matching characters.
@@ -104,7 +102,7 @@
105103 }
106104 return -1;
107105 }
108 -
 106+
109107 /**
110108 * Check whether a keypress event corresponds to the shortcut key
111109 * @param e Event object
@@ -116,7 +114,7 @@
117115 e.shiftKey == shortcutKey.shiftKey &&
118116 String.fromCharCode( e.which ).toLowerCase() == shortcutKey.key.toLowerCase();
119117 }
120 -
 118+
121119 /**
122120 * Get a description of the shortcut key, e.g. "Ctrl-M"
123121 * @return string
@@ -136,7 +134,7 @@
137135 text += shortcutKey.key.toUpperCase();
138136 return text;
139137 }
140 -
 138+
141139 /**
142140 * Change visual appearance of element (text input, textarea) according
143141 * current state of Narayam
@@ -150,7 +148,7 @@
151149 $element.removeClass( 'narayam-input' );
152150 }
153151 }
154 -
 152+
155153 /**
156154 * Keydown event handler. Handles shortcut key presses
157155 * @param e Event object
@@ -168,7 +166,7 @@
169167 }
170168 return true;
171169 }
172 -
 170+
173171 /**
174172 * Keypress event handler. This is where the real work happens
175173 * @param e Event object
@@ -177,19 +175,19 @@
178176 if ( !enabled ) {
179177 return true;
180178 }
181 -
 179+
182180 if ( e.which == 8 ) { // Backspace
183181 // Blank the keybuffer
184182 $( this ).data( 'narayamKeyBuffer', '' );
185183 return true;
186184 }
187 -
 185+
188186 // Leave non-ASCII stuff alone, as well as anything involving
189187 // Alt (except for extended keymaps), Ctrl and Meta
190188 if ( e.which < 32 || ( e.altKey && !currentScheme.extended_keyboard ) || e.ctrlKey || e.metaKey ) {
191189 return true;
192190 }
193 -
 191+
194192 var $this = $( this );
195193 var c = String.fromCharCode( e.which );
196194 // Get the current caret position. The user may have selected text to overwrite,
@@ -204,7 +202,7 @@
205203 var input = lastNChars( $this.val(), startPos, currentScheme.lookbackLength ) + c;
206204 var keyBuffer = $this.data( 'narayamKeyBuffer' );
207205 var replacement = transliterate( input, keyBuffer, e.altKey );
208 -
 206+
209207 // Update the key buffer
210208 keyBuffer += c;
211209 if ( keyBuffer.length > currentScheme.keyBufferLength ) {
@@ -212,7 +210,7 @@
213211 keyBuffer = keyBuffer.substring( keyBuffer.length - currentScheme.keyBufferLength );
214212 }
215213 $this.data( 'narayamKeyBuffer', keyBuffer );
216 -
 214+
217215 // textSelection() magic is expensive, so we avoid it as much as we can
218216 if ( replacement == input ) {
219217 return true;
@@ -222,7 +220,7 @@
223221 var divergingPos = firstDivergence( input, replacement );
224222 input = input.substring( divergingPos );
225223 replacement = replacement.substring( divergingPos );
226 -
 224+
227225 // Select and replace the text
228226 $this.textSelection( 'setSelection', {
229227 'start': startPos - input.length + 1,
@@ -233,11 +231,11 @@
234232 'replace': true,
235233 'selectPeri': false
236234 } );
237 -
 235+
238236 e.stopPropagation();
239237 return false;
240238 }
241 -
 239+
242240 /**
243241 * Focus event handler.
244242 * @param e Event object
@@ -246,7 +244,7 @@
247245 $( this ).data( 'narayamKeyBuffer', '' );
248246 changeVisual( $( this ) );
249247 }
250 -
 248+
251249 /**
252250 * Blur event handler.
253251 * @param e Event object
@@ -254,16 +252,8 @@
255253 function onblur( e ) {
256254 $( this ).removeClass( 'narayam-input' );
257255 }
258 -
259 - /**
260 - * Change handler for the scheme dropdown. Updates the current scheme
261 - * based on the new selection in the dropdown.
262 - */
263 - function updateSchemeFromSelect() {
264 - var scheme = $( this ).val();
265 - that.setScheme( scheme );
266 - }
267 -
 256+
 257+
268258 /* Public functions */
269259
270260 /**
@@ -291,7 +281,7 @@
292282 .bind( 'blur', onblur);
293283 }
294284 };
295 -
 285+
296286 /**
297287 * Enable Narayam
298288 */
@@ -299,10 +289,12 @@
300290 if ( !enabled && currentScheme !== null ) {
301291 $.cookie( 'narayam-enabled', '1', { 'path': '/', 'expires': 30 } );
302292 $( '#narayam-toggle' ).attr( 'checked', true );
 293+ $( 'li#pt-narayam').removeClass( 'narayam-inactive' );
 294+ $( 'li#pt-narayam').addClass( 'narayam-active' );
303295 enabled = true;
304296 }
305297 };
306 -
 298+
307299 /**
308300 * Disable Narayam
309301 */
@@ -310,10 +302,12 @@
311303 if ( enabled ) {
312304 $.cookie( 'narayam-enabled', '0', { 'path': '/', 'expires': 30 } );
313305 $( '#narayam-toggle' ).attr( 'checked', false );
 306+ $( 'li#pt-narayam').removeClass( 'narayam-active' );
 307+ $( 'li#pt-narayam').addClass( 'narayam-inactive' );
314308 enabled = false;
315309 }
316310 };
317 -
 311+
318312 /**
319313 * Toggle the enabled/disabled state
320314 */
@@ -324,7 +318,7 @@
325319 that.enable();
326320 }
327321 };
328 -
 322+
329323 /**
330324 * Add a transliteration scheme. Schemes whose name is not in
331325 * wgNarayamAvailableSchemes will be ignored.
@@ -373,7 +367,7 @@
374368 return false;
375369 }
376370 };
377 -
 371+
378372 /**
379373 * Change the current transliteration scheme
380374 * @param name String
@@ -382,80 +376,121 @@
383377 if ( name in schemes ) {
384378 currentScheme = schemes[name];
385379 $.cookie( 'narayam-scheme', name, { 'path': '/', 'expires': 30 } );
386 - $select.val( name );
387380 }
388381 };
389 -
 382+
390383 /**
391384 * Set up Narayam. This adds the scheme dropdown, binds the handlers
392385 * and initializes the enabled/disabled state and selected scheme
393386 * from a cookie or wgNarayamEnableByDefault
394387 */
395388 this.setup = function() {
396 - // Build scheme dropdown
397 - $select = $( '<select />' );
398389 var haveSchemes = false;
399 - for ( var scheme in schemes ) {
400 - $( '<option />' )
401 - .val( scheme )
402 - .text( mw.msg( schemes[scheme].namemsg ) )
403 - .appendTo( $select );
 390+ // Build schemes option list
 391+ var $ul = $( '<ul/>' );
 392+ for ( scheme in schemes ) {
 393+ $input = $( '<input type="radio" name="narayam-input-method" class="narayam-scheme" />' );
 394+ $input
 395+ .attr( 'id', 'narayam-' + scheme )
 396+ .val( scheme );
 397+
 398+ $( '<li/>' )
 399+ .append( $input )
 400+ .append( mw.msg( schemes[scheme].namemsg ) )
 401+ .appendTo( $ul );
404402 haveSchemes = true;
405403 }
406 - $select.change( updateSchemeFromSelect );
407 -
 404+
408405 if ( !haveSchemes ) {
409406 // No schemes available, don't show the tool
410407 return;
411408 }
412 -
 409+
 410+ // Event listener for scheme selection.
 411+ // There is a plan to add a feature that allow dynamic loading of schemes.
 412+ // So .live will be useful
 413+ $( '.narayam-scheme', $( '#narayam-menu-items > ul')[0] ).live( 'click', function(){
 414+ that.setScheme( $(this).val() );
 415+ } );
 416+
413417 // Build enable/disable checkbox and label
414418 var $checkbox = $( '<input type="checkbox" id="narayam-toggle" />' );
415419 $checkbox
416420 .attr( 'title', mw.msg( 'narayam-checkbox-tooltip' ) )
417421 .click( that.toggle );
418 -
419 - var helppage = mw.msg( 'narayam-help-page' );
 422+
420423 var $label = $( '<label for="narayam-toggle" />' );
421424 $label
422425 .text( mw.msg( 'narayam-toggle-ime', shortcutText() ) )
 426+ .prepend( $checkbox )
423427 .attr( 'title', mw.msg( 'narayam-checkbox-tooltip' ) );
 428+
 429+ var helppage = mw.msg( 'narayam-help-page' );
424430 if ( helppage ) {
425 - // Link to the help page
426 - $label.wrapInner( $( '<a />' ).attr( 'href', mw.util.wikiGetlink( helppage ) ) );
 431+ $ul.append( $( '<li class="narayam-help-link" />')
 432+ .append(
 433+ $( '<a/>' )
 434+ .text( mw.msg( 'narayam-help' ) )
 435+ .attr( 'href', mw.util.wikiGetlink( mw.msg( 'narayam-help-page' ) ))
 436+ )
 437+ );
427438 }
428 -
429 - var $checkboxAndLabel = $( '<span />' )
430 - .addClass( 'narayam-toggle-wrapper' )
431 - .append( $checkbox )
432 - .append( $label );
433 - var $spanWithEverything = $( '<span />' )
434 - .addClass( 'narayam-wrapper' )
435 - .append( $select )
436 - .append( $checkboxAndLabel );
437 -
438 - // Put the dropdown and the checkbox at the beginning of the
439 - // search form. This seems to be the most reliable way across skins.
440 - $( '#searchform' ).prepend( $spanWithEverything );
441 -
 439+
 440+ $ul.prepend( $( '<li/>' ).append( $label ) );
 441+
 442+ var $menuItems = $( '<div id="narayam-menu-items" class="menu-items" />' );
 443+ $menuItems
 444+ .append( $ul );
 445+
 446+ var $menu = $( '<div id="narayam-menu" class="narayam-menu" />');
 447+ $menu
 448+ .append(
 449+ $( '<a href="#" />' )
 450+ .text( mw.msg( 'narayam-menu' ) )
 451+ .attr( 'title', mw.msg( 'narayam-menu-tooltip' ) )
 452+ )
 453+ .append( $menuItems );
 454+
 455+ var $li = $( '<li id="pt-narayam" />');
 456+ $li
 457+ .append( $menu );
 458+
 459+ // If rtl, add to the right of top personal links, else, to the left
 460+ if($('body').is( '.rtl' ) ){
 461+ $($('#p-personal ul')[0]).append( $li );
 462+ }
 463+ else{
 464+ $($('#p-personal ul')[0]).prepend( $li );
 465+ }
 466+
 467+ // Build enable/disable checkbox and label
 468+ $checkbox = $( '<input type="checkbox" id="narayam-toggle" />' );
 469+ $checkbox
 470+ .attr( 'title', mw.msg( 'narayam-checkbox-tooltip' ) )
 471+ .click( that.toggle );
 472+
442473 // Restore state from cookies
443474 var savedScheme = $.cookie( 'narayam-scheme' );
444475 if ( savedScheme && savedScheme in schemes ) {
445476 that.setScheme( savedScheme );
 477+ $( '#narayam-' + savedScheme ).attr( 'checked', 'checked' );
446478 } else {
447 - $select.change();
 479+ $('input.narayam-scheme')[0].attr( 'checked', 'checked' );
448480 }
449481 var enabledCookie = $.cookie( 'narayam-enabled' );
450482 if ( enabledCookie == '1' || ( mw.config.get( 'wgNarayamEnabledByDefault' ) && enabledCookie !== '0' ) ) {
451483 that.enable();
452484 }
 485+ else {
 486+ $( 'li#pt-narayam').addClass( 'narayam-inactive' );
 487+ }
453488 // Renew the narayam-enabled cookie. naraym-scheme is renewed by setScheme()
454489 if ( enabledCookie ) {
455490 $.cookie( 'narayam-enabled', enabledCookie, { 'path': '/', 'expires': 30 } );
456491 }
457 -
 492+
458493 };
459 -
 494+
460495 } )();
461496
462497 } )( jQuery );
Index: trunk/extensions/Narayam/Narayam.php
@@ -132,6 +132,9 @@
133133 ),
134134 'messages' => array(
135135 'narayam-checkbox-tooltip',
 136+ 'narayam-menu',
 137+ 'narayam-menu-tooltip',
 138+ 'narayam-help',
136139 'narayam-help-page',
137140 'narayam-toggle-ime',
138141 ),

Sign-offs

UserFlagDate
Trevor Parscal (WMF)inspected22:26, 13 September 2011 (struck 22:34, 13 September 2011)

Follow-up revisions

RevisionCommit summaryAuthorDate
r96143essage=Mistakenly forgot to add in rev96142.junaidpv19:35, 2 September 2011
r96566Followup r96142: add @embedcatrope14:23, 8 September 2011
r96568Remove duplicate building of $checkbox introduced in r96142catrope14:46, 8 September 2011

Comments

#Comment by Catrope (talk | contribs)   14:34, 8 September 2011
+		var helppage = mw.msg( 'narayam-help-page' );

I know this wasn't introduced in this revision, but the help-page message should be in the content language, not in the user language. Otherwise, people who set their user language to Tamil on English Wikipedia (assuming for the moment that Narayam is enabled there), then click the Help link will be pointed to en:விக்கிப்பீடியா:தமிழ்த் தட்டச்சு which doesn't exist.

Looks OK otherwise, but the CSS will need review from a CSS guru. I'll ask Trevor.

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:31, 13 September 2011

The CSS code looks fine, but I did want to just mention that the way @noflip is being used here is perhaps more confusing than it needs to be.

Example:

/* @noflip */
div#narayam-menu {
	direction: ltr;
	float: left;
	cursor: pointer;
}
/* @noflip */
body.rtl div#narayam-menu {
	direction: rtl;
}

In this code, both rules are using @noflip, which means we aren't using CSSJanus at all, and the 2nd rule has to compensate for that by using body.rtl before the selector. If only a portion of a rule shouldn't be flipped, the better way to go is to write the rule without the properties to be preserved, then write it again with @noflip, this time only including the properties to be preserved:

Example:

div#narayam-menu {
	direction: ltr;
}
/* @noflip */
div#narayam-menu {
	float: left;
	cursor: pointer;
}

This will make the code simpler, more readable, and takes advantage of the CSSJanus system for all the other rules that should be flipped.

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:33, 13 September 2011

Also...

div#narayam-menu

Is way overkill - there shouldn't be multiple use of any IDs on a page, so adding a tag selector only adds complexity - the tag selector is rarely needed, especially when combined with classes, and should probably never be used with IDs.

Status & tagging log