r57006 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r57005‎ | r57006 | r57007 >
Date:02:45, 28 September 2009
Author:adam
Status:reverted (Comments)
Tags:
Comment:
addressing feedback on the watch/unwatch javascript changes introduced in r56924
Modified paths:
  • /trunk/phase3/skins/common/ajaxwatch.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/ajaxwatch.js
——@@ -25,27 +25,30 @@
2626 wgAjaxWatch.imgBasePath = ""; // base img path derived from icons on load
2727
2828 wgAjaxWatch.setLinkText = function(newText) {
29 - if(wgAjaxWatch.iconMode){
30 - for (i = 0; i < wgAjaxWatch.watchLinks.length; i++) {
 29+ if( wgAjaxWatch.iconMode ) {
 30+ for ( i = 0; i < wgAjaxWatch.watchLinks.length; i++ ) {
3131 wgAjaxWatch.watchLinks[i].firstChild.alt = newText;
32 - if(newText==wgAjaxWatch.watchingMsg||newText==wgAjaxWatch.unwatchingMsg){
33 - wgAjaxWatch.watchLinks[i].firstChild.src = wgAjaxWatch.imgBasePath+"/skins/common/images/spinner.gif";
34 - }else if(newText==wgAjaxWatch.watchMsg){
35 - wgAjaxWatch.watchLinks[i].firstChild.src = wgAjaxWatch.imgBasePath+"/skins/vector/images/watch_off.gif";
36 - }else if(newText==wgAjaxWatch.unwatchMsg){
37 - wgAjaxWatch.watchLinks[i].firstChild.src = wgAjaxWatch.imgBasePath+"/skins/vector/images/watch_on.gif";
 32+ if( newText==wgAjaxWatch.watchingMsg || newText==wgAjaxWatch.unwatchingMsg ) {
 33+ wgAjaxWatch.watchLinks[i].firstChild.src = wgAjaxWatch.imgBasePath
 34+ + "/common/images/spinner.gif";
 35+ } else if( newText==wgAjaxWatch.watchMsg ) {
 36+ wgAjaxWatch.watchLinks[i].firstChild.src = wgAjaxWatch.imgBasePath
 37+ + "/vector/images/watch_off.gif";
 38+ } else if( newText==wgAjaxWatch.unwatchMsg ) {
 39+ wgAjaxWatch.watchLinks[i].firstChild.src = wgAjaxWatch.imgBasePath
 40+ + "/vector/images/watch_on.gif";
3841 }
3942 }
40 - }else{
41 - for (i = 0; i < wgAjaxWatch.watchLinks.length; i++) {
42 - changeText(wgAjaxWatch.watchLinks[i], newText);
 43+ } else{
 44+ for ( i = 0; i < wgAjaxWatch.watchLinks.length; i++ ) {
 45+ changeText( wgAjaxWatch.watchLinks[i], newText );
4346 }
4447 }
4548 };
4649
4750 wgAjaxWatch.setLinkID = function(newId) {
4851 // We can only set the first one
49 - wgAjaxWatch.watchLinks[0].setAttribute( 'id', newId );
 52+ wgAjaxWatch.watchLinks[0].parentNode.setAttribute( 'id', newId );
5053 akeytt(newId); // update tooltips for Monobook
5154 };
5255
——@@ -130,20 +133,20 @@
131134
132135 var el1 = document.getElementById("ca-unwatch");
133136 var el2 = null;
134 - if (!el1) {
 137+ if ( !el1 ) {
135138 el1 = document.getElementById("mw-unwatch-link1");
136139 el2 = document.getElementById("mw-unwatch-link2");
137140 }
138 - if(el1) {
 141+ if( el1 ) {
139142 wgAjaxWatch.watching = true;
140143 } else {
141144 wgAjaxWatch.watching = false;
142145 el1 = document.getElementById("ca-watch");
143 - if (!el1) {
 146+ if ( !el1 ) {
144147 el1 = document.getElementById("mw-watch-link1");
145148 el2 = document.getElementById("mw-watch-link2");
146149 }
147 - if(!el1) {
 150+ if( !el1 ) {
148151 wgAjaxWatch.supported = false;
149152 return;
150153 }
——@@ -151,17 +154,20 @@
152155
153156 // If we're using the icon, add rollover affects
154157 try{
155 - if(el1.firstChild.firstChild.tagName.match(/img/i)){
156 - wgAjaxWatch.iconMode = true;
157 - wgAjaxWatch.imgBasePath = el1.firstChild.firstChild.src.replace(/\/skins\/vector\/images\/watch_(off|on).gif/, "");
158 - el1.firstChild.onmouseover = function(e){
159 - this.firstChild.src = (wgAjaxWatch.watching ? this.firstChild.src.replace(/_on/, "_off") : this.firstChild.src.replace(/_off/, "_on"));
 158+ if( el1.firstChild.firstChild.tagName.match( /img/i ) ) {
 159+ wgAjaxWatch.iconMode = true;
 160+ wgAjaxWatch.imgBasePath = el1.firstChild.firstChild.src
 161+ .replace( /\/vector\/images\/watch_(off|on).gif/, "" );
 162+ el1.firstChild.onmouseover = function( e ) {
 163+ this.firstChild.src = wgAjaxWatch.imgBasePath
 164+ + "/vector/images/watch_over.gif";
160165 }
161 - el1.firstChild.onmouseout = function(e){
162 - this.firstChild.src = (wgAjaxWatch.watching ? this.firstChild.src.replace(/_off/, "_on") : this.firstChild.src.replace(/_on/, "_off"));
 166+ el1.firstChild.onmouseout = function( e ) {
 167+ this.firstChild.src = wgAjaxWatch.imgBasePath
 168+ + "/vector/images/watch_" + ( wgAjaxWatch.watching ? "on.gif" : "off.gif");
163169 }
164170 }
165 - }catch(e){
 171+ } catch( e ) {
166172 // not using the icon
167173 }
168174

Follow-up revisions

RevisionCommit summaryAuthorDate
r57125Revert r56924 (new watch icon and color changes for Vector) and followups r57...catrope17:58, 30 September 2009

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r56924Moving watch/unwatch action out of the drop downadam18:04, 25 September 2009

Comments

#Comment by Catrope (talk | contribs)   07:42, 28 September 2009

You're still using spaces for indentation in some places; it looks like you were using 2 spaces for indentation and replaced all units of 4 spaces by tabs. Our policy is not to use spaces for indentation ever; only use tabs. If you don't like the way that looks, set your editor's tab width to 4 or 2.

#Comment by Catrope (talk | contribs)   17:37, 28 September 2009

Fixed this in r57013.

#Comment by Adammiller~mediawikiwiki (talk | contribs)   19:05, 28 September 2009

My editor was set to use soft tabs (spaces) by default. I changed it when I saw the comment pointing me to the coding conventions the other day. Thanks for cleaning up the strays.

Status & tagging log