r71601 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71600‎ | r71601 | r71602 >
Date:23:04, 24 August 2010
Author:tkriplean
Status:deferred (Comments)
Tags:
Comment:
reflect compatible with IE8
Modified paths:
  • /trunk/extensions/Reflect/client/css/reflect.css (modified) (history)
  • /trunk/extensions/Reflect/client/js/reflect.js (modified) (history)
  • /trunk/extensions/Reflect/client/js/reflect.study.js (modified) (history)
  • /trunk/extensions/Reflect/client/js/third_party/jquery.color.js (modified) (history)
  • /trunk/extensions/Reflect/client/js/third_party/json2.js (modified) (history)
  • /trunk/extensions/Reflect/client/templates/templates.html (modified) (history)

Diff [purge]

Index: trunk/extensions/Reflect/client/css/reflect.css
——@@ -220,6 +220,7 @@
221221 height: 60px;
222222 padding: 4px 2px;
223223 width:100%;
 224+ overflow: auto;
224225 }
225226
226227 #reflected .rf_comment_summary tr.submit_footer {
——@@ -464,7 +465,7 @@
465466
466467 #reflected div.new_bullet_prompt {
467468 position:absolute;
468 - top:-7px;
 469+ top:-9px;
469470 z-index:50;
470471 width:92%;
471472 margin-left:3px;
Index: trunk/extensions/Reflect/client/js/reflect.js
——@@ -156,7 +156,9 @@
157157
158158 text = jQuery.trim(
159159 text.substring( 0,
160 - text.indexOf( "<span class=\"username\">" ) ) );
 160+ text
 161+ .toLowerCase()
 162+ .indexOf( "<span class=" ) ) );
161163 function add_highlights () {
162164 var el_id = $j( this ).attr( 'id' ).substring( 9 );
163165 highlights.push( {
——@@ -859,7 +861,6 @@
860862 var highlight = bullet_obj.enter_highlight_state();
861863
862864 highlight.find( 'td:first' ).addClass( 'connect_directions' )
863 - .text( 'Please click the relevant sentences' )
864865 .css( 'color', Reflect.utils.get_background_color( highlight ) )
865866 .css( 'background-color', Reflect.utils
866867 .get_inverted_background_color( highlight, function ( c ) {
——@@ -992,39 +993,38 @@
993994 return Reflect.current_user;
994995 },
995996
996 - get_background_color : function ( node ) {
997 - var current_p = $j( node ),
998 - rgbString = "transparent";
999 - while ( current_p
1000 - && (rgbString == "transparent"
1001 - || rgbString == "initial"
1002 - || rgbString == 'rgba(0, 0, 0, 0)') )
1003 - {
1004 - rgbString = current_p.parent().css( 'background-color' );
1005 - current_p = current_p.parent();
 997+ get_background_color : function ( node, no_convert ) {
 998+ col = $j.getColor(node[0], 'background-color');
 999+ if ( !no_convert ) {
 1000+ var new_col = "#";
 1001+ for ( var i = 0; i <= 2; ++i) {
 1002+ var new_color = col[i].toString( 16 );
 1003+ if ( new_color.length == 1 ) {
 1004+ new_color = '0' + new_color;
 1005+ }
 1006+ new_col += new_color;
 1007+ }
 1008+ col = new_col;
10061009 }
1007 - if ( !rgbString ) {
1008 - rgbString = 'rgb(0, 0, 0)';
1009 - }
1010 - return rgbString;
 1010+ return col
10111011 },
10121012
10131013 get_inverted_background_color : function ( node, color_convert ) {
10141014 try {
1015 - var rgbString = Reflect.utils.get_background_color( node ),
1016 - parts = rgbString.match( /^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/ ),
 1015+ var rgbString = Reflect.utils.get_background_color( node, true ),
10171016 res = '#';
1018 - for ( var i = 1; i <= 3; ++i) {
1019 - var color = parseInt( parts[i] ),
1020 - new_color = color_convert( color );
1021 - parts[i] = new_color.toString( 16 );
1022 - if ( parts[i].length == 1 ) {
1023 - parts[i] = '0' + parts[i];
 1017+
 1018+ for ( var i = 0; i <= 2; ++i) {
 1019+ var new_color = color_convert( rgbString[i] )
 1020+ .toString( 16 );
 1021+ if ( new_color.length == 1 ) {
 1022+ new_color = '0' + new_color;
10241023 }
1025 - res += parts[i];
 1024+ res += new_color;
10261025 }
10271026 return res;
10281027 } catch ( err ) {
 1028+ console.log(err);
10291029 return '#555';
10301030 }
10311031 }
——@@ -1247,7 +1247,8 @@
12481248 if ( modify ) {
12491249 text = this.elements.bullet_text.html();
12501250 text = $j.trim( text.substring( 0, text
1251 - .indexOf( "<span class=\"username\">" ) ) );
 1251+ .toLowerCase()
 1252+ .indexOf( "<span class=" ) ) );
12521253 }
12531254
12541255 var template_vars = {
——@@ -1411,7 +1412,8 @@
14121413 if ( modify ) {
14131414 var text = this.elements.response_text.html();
14141415 this.options.text = $j.trim( text.substring( 0, text
1415 - .indexOf( "<span class=\"username\">" ) ) );
 1416+ .toLowerCase()
 1417+ .indexOf( "<span class=" ) ) );
14161418 }
14171419
14181420 this._build_prompt();
——@@ -1600,12 +1602,13 @@
16011603 if ( Reflect.config.study ) {
16021604 Reflect.study.load_surveys();
16031605 }
1604 -
16051606 } );
16061607 } );
16071608 }
16081609 };
16091610
16101611 $j( document ).ready( function () {
 1612+ $j.ajaxSetup({ cache: false });
 1613+
16111614 Reflect.init();
16121615 } );
Index: trunk/extensions/Reflect/client/js/reflect.study.js
——@@ -16,6 +16,24 @@
1717 // ///////////////////
1818 }
1919
 20+
 21+function filter(a, fun){
 22+ var len = a.length >>> 0;
 23+ if (typeof fun != "function")
 24+ throw new TypeError();
 25+
 26+ var res = [];
 27+ var thisp = arguments[1];
 28+ for (var i = 0; i < len; i++) {
 29+ if (i in a) {
 30+ var val = a[i]; // in case fun mutates this
 31+ if (fun.call(thisp, val, i, a))
 32+ res.push(val);
 33+ }
 34+ }
 35+ return res;
 36+}
 37+
2038 var $j = jQuery.noConflict();
2139
2240 Reflect.study = {
——@@ -25,8 +43,13 @@
2644 user = Reflect.utils.get_logged_in_user();
2745 // in a but not in b
2846 function relative_complement ( a, b ) {
29 - return a.filter( function ( elem ) {
30 - return b.indexOf( elem ) == -1;
 47+ return filter(a,function ( elem ) {
 48+ for ( var i in b ) {
 49+ if ( b[i] == elem ) {
 50+ return false;
 51+ }
 52+ }
 53+ return true;
3154 } );
3255 }
3356 $j( '.bullet' ).each( function () {
——@@ -43,18 +66,19 @@
4467 // for each candidate bullet NOT in data, lay down
4568 // appropriate survey
4669 var needs_surveys = relative_complement( bullets, data );
47 - for ( var i in needs_surveys) {
48 - var bullet = $j( '#bullet-' + needs_surveys[i] )
49 - .data( 'bullet' );
50 - if ( bullet.comment.user == user
51 - && bullet.responses.length > 0
52 - && bullet.responses[0].data( 'response' ).id ) {
53 - Reflect.study.new_bullet_reaction_survey(
54 - bullet, bullet.comment, bullet.$elem );
55 - } else if ( bullet.user == user ) {
56 - Reflect.study.new_bullet_survey(
57 - bullet, bullet.comment, bullet.$elem );
58 - }
 70+ for (var i in needs_surveys) {
 71+ try {
 72+ var bullet = $j('#bullet-' + needs_surveys[i]).data('bullet');
 73+ if (bullet.comment.user == user &&
 74+ bullet.responses.length > 0 &&
 75+ bullet.responses[0].data('response').id) {
 76+ Reflect.study.new_bullet_reaction_survey(bullet, bullet.comment, bullet.$elem);
 77+ }
 78+ else
 79+ if (bullet.user == user) {
 80+ Reflect.study.new_bullet_survey(bullet, bullet.comment, bullet.$elem);
 81+ }
 82+ } catch( err ) {}
5983 }
6084
6185 } );
Index: trunk/extensions/Reflect/client/js/third_party/json2.js
——@@ -4,7 +4,7 @@
55 * No warranty expressed or implied. Use at your own risk.
66 * See http://www.JSON.org/js.html
77 */
8 -if(!this.JSON){JSON=function(){function f(n){return n<10?'0'+n:n;}
 8+JSON=function(){function f(n){return n<10?'0'+n:n;}
99 Date.prototype.toJSON=function(){return this.getUTCFullYear()+'-'+
1010 f(this.getUTCMonth()+1)+'-'+
1111 f(this.getUTCDate())+'T'+
——@@ -21,4 +21,4 @@
2222 return{stringify:stringify,parse:function(text,filter){var j;function walk(k,v){var i,n;if(v&&typeof v==='object'){for(i in v){if(Object.prototype.hasOwnProperty.apply(v,[i])){n=walk(i,v[i]);if(n!==undefined){v[i]=n;}}}}
2323 return filter(k,v);}
2424 if(/^[\],:{}\s]*$/.test(text.replace(/\\./g,'@').replace(/"[^"\\\n\r]*"|true|false|null|-?\d+(?:\.\d*)?(?:[eE][+\-]?\d+)?/g,']').replace(/(?:^|:|,)(?:\s*\[)+/g,''))){j=eval('('+text+')');return typeof filter==='function'?walk('',j):j;}
25 -throw new SyntaxError('parseJSON');}};}();}
\ No newline at end of file
 25+throw new SyntaxError('parseJSON');}};}();
\ No newline at end of file
Index: trunk/extensions/Reflect/client/js/third_party/jquery.color.js
——@@ -123,4 +123,7 @@
124124 yellow:[255,255,0]
125125 };
126126
 127+ jQuery.extend({getColor: getColor});
127128 })(jQuery);
 129+
 130+
Index: trunk/extensions/Reflect/client/templates/templates.html
——@@ -2,28 +2,28 @@
33 <![CDATA[
44 <div class="new_bullet_prompt">
55 <ul>
6 - <li class=elicitation>Add a point that <%= this.commenter %> makes...</li>
7 - <li class=count>
8 - <a title="Please limit your bullet to 140 characters or less."><span class=count></span></a>
 6+ <li class="elicitation">Add a point that <%= this.commenter %> makes...</li>
 7+ <li class="count">
 8+ <a title="Please limit your bullet to 140 characters or less."><span class="count"></span></a>
99 </li>
1010 </ul>
1111 </div>
1212 <table class="new_bullet_wrapper reflect">
1313 <tr>
14 - <td class=new_bullet_text_wrap>
15 - <textarea class=new_bullet_text><% if (this.txt) %><%= this.txt %></textarea>
 14+ <td class="new_bullet_text_wrap">
 15+ <textarea class="new_bullet_text"><% if (this.txt) %><%= this.txt %></textarea>
1616 </td>
17 - <td class=submit_buttons>
 17+ <td class="submit_buttons">
1818 <div><button class="cancel_bullet"><img src="<%= this.media_dir %>/cancel2.png" ></button></div>
1919 </td>
2020 </tr>
21 - <tr class=submit_footer>
 21+ <tr class="submit_footer">
2222 <td>
2323 <ul>
24 - <li class=submit>
 24+ <li class="submit">
2525 <button class="bullet_submit">Done</button>
2626 </li>
27 - <li class=be_neutral>
 27+ <li class="be_neutral">
2828 <a title="Someone else reading your summary bullet point should not be able to tell whether YOU agree or disagree it.">
2929 don't respond, <span class="big_word">summarize</span>
3030 </a>
——@@ -65,7 +65,7 @@
6666 <script type="text/html" id="reflect_template_new_response">
6767 <![CDATA[
6868 <ul class="rebutt_list" >
69 - <li class=img>
 69+ <li class="img">
7070 <% if(this.sig == "2") { %>
7171 <span class="response_yes">+</span>
7272 <% } else if(this.sig == "1") { %>
——@@ -74,10 +74,10 @@
7575 <span class="response_no">-</span>
7676 <% } %>
7777 </li>
78 - <li class=rebutt_txt><%= this.text %> <span class=username><a class=user><%= this.user %></a></span></li>
 78+ <li class="rebutt_txt"><%= this.text %> <span class="username"><a class=user><%= this.user %></a></span></li>
7979 </ul>
80 - <ul class=response_footer_wrapper>
81 - <li class=modify_operation>
 80+ <ul class="response_footer_wrapper">
 81+ <li class="modify_operation">
8282 <button class="modify">
8383 <a title="modify">
8484 <img class="base" src="<%= this.media_dir %>/edit.png">
——@@ -87,8 +87,8 @@
8888 </li>
8989 <li class="delete_operation">
9090 <button class="delete">
91 - <img class=base src="<%= this.media_dir %>/delete_gray.png">
92 - <img title="Delete" class=hover src="<%= this.media_dir %>/delete_red.png">
 91+ <img class="base" src="<%= this.media_dir %>/delete_gray.png">
 92+ <img title="Delete" class="hover" src="<%= this.media_dir %>/delete_red.png">
9393 </button>
9494 </li>
9595 <li class="dispute_operation">
——@@ -96,7 +96,7 @@
9797 <img class="base" src="<%= this.media_dir %>/comment-flag.png">
9898 <img class=hover src="<%= this.media_dir %>/comment-flag-hover.png">
9999 </span>
100 - <ul class=bullet_report_problem>
 100+ <ul class="bullet_report_problem">
101101 <li>not written neutrally</li>
102102 </ul>
103103 </li>
——@@ -120,7 +120,7 @@
121121 <ul class="response_def">
122122 <li class="response_prompt">
123123 <label class="prompt">Did you make this point?</label>
124 - <ul class=response_eval>
 124+ <ul class="response_eval">
125125 <li><input type="radio" name="accurate-<%=this.bullet_id %>" value="2" <% if(this.sig == "2"){ %> CHECKED <% } %>>Yes</li>
126126 <li><input type="radio" name="accurate-<%=this.bullet_id %>" value="1" <% if(this.sig == "1"){ %> CHECKED <% } %>>Kind of...</li>
127127 <li><input type="radio" name="accurate-<%=this.bullet_id %>" value="0" <% if(this.sig == "0"){ %> CHECKED <% } %>>No</li>
——@@ -129,10 +129,10 @@
130130 <li class=response_dialog>
131131 <table class="new_bullet_wrapper reflect">
132132 <tr>
133 - <td class=new_bullet_text_wrap>
 133+ <td class="new_bullet_text_wrap">
134134 <textarea class="new_bullet_text"><% if(this.text) %><%= this.text %></textarea>
135135 </td>
136 - <td class=submit_buttons>
 136+ <td class="submit_buttons">
137137 <div>
138138 <button class="cancel_bullet">
139139 <img src="<%= this.media_dir %>/cancel2.png">
——@@ -140,13 +140,13 @@
141141 </div>
142142 </td>
143143 </tr>
144 - <tr class=submit_footer>
 144+ <tr class="submit_footer">
145145 <td>
146146 <ul>
147 - <li class=submit>
 147+ <li class="submit">
148148 <button class="bullet_submit">Done</button>
149149 </li>
150 - <li class=count>
 150+ <li class="count">
151151 <a title="Please limit your response to 140 characters or less.">
152152 <span class="count"></span>
153153 </a>
——@@ -169,29 +169,29 @@
170170 <%= this.bullet_text %> <span class="username"><a class="user"><%= this.user %></a></span>
171171 </li>
172172 <li class="bullet_footer_wrapper">
173 - <ul class=bullet_operations>
174 - <li class=modify_operation>
175 - <button class=modify>
 173+ <ul class="bullet_operations">
 174+ <li class="modify_operation">
 175+ <button class="modify">
176176 <a title="modify">
177 - <img class=base src="<%= this.media_dir %>/edit.png"></img>
178 - <img title="Modify" class=hover src="<%= this.media_dir %>/edit_hover.png"></img>
 177+ <img class="base" src="<%= this.media_dir %>/edit.png"></img>
 178+ <img title="Modify" class="hover" src="<%= this.media_dir %>/edit_hover.png"></img>
179179 </a>
180180 </button>
181181 </li>
182 - <li class=delete_operation>
183 - <button class=delete>
 182+ <li class="delete_operation">
 183+ <button class="delete">
184184 <a title="delete">
185 - <img class=base src="<%= this.media_dir %>/delete_gray.png"></img>
186 - <img title="Delete" class=hover src="<%= this.media_dir %>/delete_red.png"></img>
 185+ <img class="base" src="<%= this.media_dir %>/delete_gray.png"></img>
 186+ <img title="Delete" class="hover" src="<%= this.media_dir %>/delete_red.png"></img>
187187 </a>
188188 </button>
189189 </li>
190 - <li class=dispute_operation>
 190+ <li class="dispute_operation">
191191 <span class="bullet_prompt_problem">
192 - <img class=base src="<%= this.media_dir %>/comment-flag.png"></img>
193 - <img class=hover src="<%= this.media_dir %>/comment-flag-hover.png"></img>
 192+ <img class="base" src="<%= this.media_dir %>/comment-flag.png"></img>
 193+ <img class="hover" src="<%= this.media_dir %>/comment-flag-hover.png"></img>
194194 </span>
195 - <ul class=bullet_report_problem>
 195+ <ul class="bullet_report_problem">
196196 <li class="flag" name="input">not a summary</li>
197197 <li class="flag" name="neutral">not written neutrally</li>
198198 <li class="flag" name="accurate"><a class=user><%= this.commenter %></a> didn't say this</li>

Comments

#Comment by Siebrand (talk | contribs)   23:19, 24 August 2010

Please review i18n. There is a lot of hard coded text visible in this commit. If you address i18n properly in the early stages of development, it will not be a burden later on...

Random sample: 

+         <li class="elicitation">Add a point that <%= this.commenter %> makes...</li>
+         <li class="count">
+               <a title="Please limit your bullet to 140 characters or 
+ less."><span class="count"></span></a>
#Comment by Tkriplean (talk | contribs)   17:24, 26 August 2010

Thanks, I'll look into how to do this after I get back from vacation

#Comment by Brion VIBBER (talk | contribs)   00:14, 28 January 2011

Changing from fixme to deferred:

  • the called-out i18n issue is not part of revision -- the hardcoded English text was not introduced here
  • this extension is not in use on Wikimedia, so minor issues are not blockers for MediaWiki deployment or release
  • it's not entirely clear that the extension currently works at all (no README or usage directions on Extension:Reflect, no obvious UI visible when running on trunk), making a fix of this issue even less important for release finalization.

I've broken the i18n issue out to bugzilla:26998.

#Comment by MarkAHershberger (talk | contribs)   00:42, 28 January 2011

See Bug#26999

#Comment by MarkAHershberger (talk | contribs)   00:59, 28 January 2011

Also https://bugzilla.wikimedia.org/show_bug.cgi?id=26998 and others.

Status & tagging log