r57307 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r57306‎ | r57307 | r57308 >
Date:21:04, 2 October 2009
Author:catrope
Status:ok (Comments)
Tags:
Comment:
* EditToolbar: (bug 20917) Search&Replace dialog doesn't handle empty "Search for:" field well; show a message instead
* (bug 20916) Make the text fields in the table dialog a bit wider so three digits actually fit on Safari
Modified paths:
  • /trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.hooks.php (modified) (history)
  • /trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.i18n.php (modified) (history)
  • /trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.js (modified) (history)
  • /trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.php (modified) (history)

Diff [purge]

Index: trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.hooks.php
——@@ -120,6 +120,7 @@
121121 'edittoolbar-tool-replace-close',
122122 'edittoolbar-tool-replace-nomatch',
123123 'edittoolbar-tool-replace-success',
 124+ 'edittoolbar-tool-replace-emptysearch',
124125 /* Special Characters Section */
125126 'edittoolbar-section-characters',
126127 'edittoolbar-characters-page-latin',
Index: trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.js
——@@ -1084,10 +1084,10 @@
10851085 </tr></table><table><tr>\
10861086 <td class="label"><label for="edittoolbar-table-dimensions-columns"\
10871087 rel="edittoolbar-tool-table-dimensions-columns"></label></td>\
1088 - <td><input type="text" id="edittoolbar-table-dimensions-columns" size="2" /></td>\
 1088+ <td><input type="text" id="edittoolbar-table-dimensions-columns" size="3" /></td>\
10891089 <td class="label"><label for="edittoolbar-table-dimensions-rows"\
10901090 rel="edittoolbar-tool-table-dimensions-rows"></label></td>\
1091 - <td><input type="text" id="edittoolbar-table-dimensions-rows" size="2" /></td>\
 1091+ <td><input type="text" id="edittoolbar-table-dimensions-rows" size="3" /></td>\
10921092 </tr></table></fieldset>',
10931093 init: function() {
10941094 $j(this).find( '[rel]' ).each( function() {
——@@ -1184,6 +1184,7 @@
11851185 <div id="edittoolbar-replace-message">\
11861186 <div id="edittoolbar-replace-nomatch" rel="edittoolbar-tool-replace-nomatch"></div>\
11871187 <div id="edittoolbar-replace-success"></div>\
 1188+ <div id="edittoolbar-replace-emptysearch" rel="edittoolbar-tool-replace-emptysearch"></div>\
11881189 </div>\
11891190 <fieldset><table><tr>\
11901191 <td><label for="edittoolbar-replace-search" rel="edittoolbar-tool-replace-search"></label></td>\
——@@ -1214,8 +1215,12 @@
12151216
12161217 // TODO: Find a cleaner way to share this function
12171218 $j(this).data( 'replaceCallback', function( mode ) {
1218 - $j( '#edittoolbar-replace-nomatch, #edittoolbar-replace-success' ).hide();
 1219+ $j( '#edittoolbar-replace-nomatch, #edittoolbar-replace-success, #edittoolbar-replace-emptysearch' ).hide();
12191220 var searchStr = $j( '#edittoolbar-replace-search' ).val();
 1221+ if ( searchStr == '' ) {
 1222+ $j( '#edittoolbar-replace-emptysearch' ).show();
 1223+ return;
 1224+ }
12201225 var replaceStr = $j( '#edittoolbar-replace-replace' ).val();
12211226 var flags = '';
12221227 var matchCase = $j( '#edittoolbar-replace-case' ).is( ':checked' );
——@@ -1288,7 +1293,7 @@
12891294 open: function() {
12901295 $j(this).data( 'offset', 0 );
12911296 $j( '#edittoolbar-replace-search' ).focus();
1292 - $j( '#edittoolbar-replace-nomatch, #edittoolbar-replace-success' ).hide();
 1297+ $j( '#edittoolbar-replace-nomatch, #edittoolbar-replace-success, #edittoolbar-replace-emptysearch' ).hide();
12931298 if ( !( $j(this).data( 'dialogkeypressset' ) ) ) {
12941299 $j(this).data( 'dialogkeypressset', true );
12951300 // Execute the action associated with the first button
Index: trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.i18n.php
——@@ -114,6 +114,7 @@
115115 'edittoolbar-tool-replace-close' => 'Cancel',
116116 'edittoolbar-tool-replace-nomatch' => 'Your search did not match anything.',
117117 'edittoolbar-tool-replace-success' => '$1 replacement(s) made.',
 118+ 'edittoolbar-tool-replace-emptysearch' => 'You did not enter anything to search for.',
118119 /* Special characters Section */
119120 'edittoolbar-section-characters' => 'Special characters',
120121 'edittoolbar-characters-page-latin' => 'Latin',
Index: trunk/extensions/UsabilityInitiative/EditToolbar/EditToolbar.php
——@@ -19,7 +19,7 @@
2020 /* Configuration */
2121
2222 // Bump the version number every time you change any of the .css/.js files
23 -$wgEditToolbarStyleVersion = 47;
 23+$wgEditToolbarStyleVersion = 48;
2424
2525 // Set this to true to simply override the stock toolbar for everyone
2626 $wgEditToolbarGlobalEnable = false;

Follow-up revisions

RevisionCommit summaryAuthorDate
r57426EditToolbar: (bug 20916) Make the textboxes in the table dialog one char longercatrope14:46, 6 October 2009
r57719wmf-deployment: Merge babaco fixes from trunk...catrope20:20, 14 October 2009

Comments

#Comment by Nikerabbit (talk | contribs)   14:46, 3 October 2009

Isn't this bad usability? Just don't make it possible for people to do wrong things.

#Comment by Catrope (talk | contribs)   10:33, 4 October 2009

Isn't that exactly what this revision /does/? It displays a warning message when you try to search for or replace an empty string, instead of producing unpredictable results.

#Comment by Nikerabbit (talk | contribs)   07:13, 5 October 2009

I'm arguing that the error message is not needed. Compare with the incremental in-page search in Firefox for example. It just doesn't let you search with empty string.

#Comment by Catrope (talk | contribs)   09:25, 5 October 2009

Incremental search is different from entering a search term and clicking "Search". Users may get confused when they click a button and nothing happens.

#Comment by Brion VIBBER (talk | contribs)   18:00, 14 October 2009

I'd recommend just keeping the search button disabled until there's something in the field.

#Comment by Brion VIBBER (talk | contribs)   18:36, 14 October 2009

Ok objection withdrawn -- the behavior is consistent with searching for text that returns no results. I'd gotten the impression above that it was an alert()-style popup, but it's a one-line message within the dialog.

(Did find some keyboard accessibility problems while testing this though!)

Status & tagging log