r56924 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r56923‎ | r56924 | r56925 >
Date:18:04, 25 September 2009
Author:adam
Status:reverted (Comments)
Tags:
Comment:
Moving watch/unwatch action out of the drop down
Modified paths:
  • /trunk/phase3/skins/Vector.php (modified) (history)
  • /trunk/phase3/skins/common/ajaxwatch.js (modified) (history)
  • /trunk/phase3/skins/vector/images/watch_off.gif (added) (history)
  • /trunk/phase3/skins/vector/images/watch_on.gif (added) (history)
  • /trunk/phase3/skins/vector/main-ltr.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/ajaxwatch.js
——@@ -21,11 +21,26 @@
2222 wgAjaxWatch.inprogress = false; // ajax request in progress
2323 wgAjaxWatch.timeoutID = null; // see wgAjaxWatch.ajaxCall
2424 wgAjaxWatch.watchLinks = []; // "watch"/"unwatch" links
 25+wgAjaxWatch.iconMode = false; // new icon driven functionality
 26+wgAjaxWatch.imgBasePath = ""; // base img path derived from icons on load
2527
2628 wgAjaxWatch.setLinkText = function(newText) {
27 - for (i = 0; i < wgAjaxWatch.watchLinks.length; i++) {
28 - changeText(wgAjaxWatch.watchLinks[i], newText);
29 - }
 29+ if(wgAjaxWatch.iconMode){
 30+ for (i = 0; i < wgAjaxWatch.watchLinks.length; i++) {
 31+ 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";
 38+ }
 39+ }
 40+ }else{
 41+ for (i = 0; i < wgAjaxWatch.watchLinks.length; i++) {
 42+ changeText(wgAjaxWatch.watchLinks[i], newText);
 43+ }
 44+ }
3045 };
3146
3247 wgAjaxWatch.setLinkID = function(newId) {
——@@ -112,6 +127,7 @@
113128 wgAjaxWatch.onLoad = function() {
114129 // This document structure hardcoding sucks. We should make a class and
115130 // toss all this out the window.
 131+
116132 var el1 = document.getElementById("ca-unwatch");
117133 var el2 = null;
118134 if (!el1) {
——@@ -132,7 +148,24 @@
133149 return;
134150 }
135151 }
 152+
 153+ // If we're using the icon, add rollover affects
 154+ 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"));
 160+ }
 161+ el1.firstChild.onmouseout = function(e){
 162+ this.firstChild.src = (wgAjaxWatch.watching ? this.firstChild.src.replace(/_off/, "_on") : this.firstChild.src.replace(/_on/, "_off"));
 163+ }
 164+ }
 165+ }catch(e){
 166+ // not using the icon
 167+ }
136168
 169+
137170 // The id can be either for the parent (Monobook-based) or the element
138171 // itself (non-Monobook)
139172 wgAjaxWatch.watchLinks.push( el1.tagName.toLowerCase() == "a"
Index: trunk/phase3/skins/Vector.php
——@@ -59,7 +59,7 @@
6060 * @private
6161 */
6262 function buildNavigationUrls() {
63 - global $wgContLang, $wgLang, $wgOut, $wgUser, $wgRequest, $wgArticle;
 63+ global $wgContLang, $wgLang, $wgOut, $wgUser, $wgRequest, $wgArticle, $wgStylePath;
6464 global $wgDisableLangConversion;
6565
6666 wfProfileIn( __METHOD__ );
——@@ -285,21 +285,23 @@
286286 if( $this->loggedin ) {
287287 // Checks if the user is watching this page
288288 if( !$this->mTitle->userIsWatching() ) {
289 - // Adds watch action link
290 - $links['actions']['watch'] = array(
 289+ // Adds watch view link
 290+ $links['views']['watch'] = array(
291291 'class' =>
292292 ( $action == 'watch' or $action == 'unwatch' ) ?
293293 'selected' : false,
294294 'text' => wfMsg( 'watch' ),
 295+ 'img' => "{$wgStylePath}/vector/images/watch_off.gif",
295296 'href' => $this->mTitle->getLocalUrl( 'action=watch' )
296297 );
297298 } else {
298 - // Adds unwatch action link
299 - $links['actions']['unwatch'] = array(
 299+ // Adds unwatch view link
 300+ $links['views']['unwatch'] = array(
300301 'class' =>
301302 ($action == 'unwatch' or $action == 'watch') ?
302303 'selected' : false,
303304 'text' => wfMsg( 'unwatch' ),
 305+ 'img' => "{$wgStylePath}/vector/images/watch_on.gif",
304306 'href' => $this->mTitle->getLocalUrl( 'action=unwatch' )
305307 );
306308 }
——@@ -722,7 +724,7 @@
723725 <h5><?php $this->msg('views') ?></h5>
724726 <ul <?php $this->html('userlangattributes') ?>>
725727 <?php foreach ($this->data['view_urls'] as $key => $link ): ?>
726 - <li<?php echo $link['attributes'] ?>><a href="<?php echo htmlspecialchars( $link['href'] ) ?>" <?php echo $link['key'] ?>><span><?php echo htmlspecialchars( $link['text'] ) ?></span></a></li>
 728+ <li<?php echo $link['attributes'] ?>><a href="<?php echo htmlspecialchars( $link['href'] ) ?>" <?php echo $link['key'] ?>><?php echo (array_key_exists('img',$link) ? '<img src="'.$link['img'].'" alt="'.$link['text'].'" />' : '<span>'.htmlspecialchars( $link['text'] ).'</span>') ?></a></li>
727729 <?php endforeach; ?>
728730 </ul>
729731 </div>
Index: trunk/phase3/skins/vector/images/watch_off.gif
Cannot display: file marked as a binary type.
svn:mime-type = application/octet-stream
Property changes on: trunk/phase3/skins/vector/images/watch_off.gif
___________________________________________________________________
Name: svn:mime-type
730732 + application/octet-stream
Index: trunk/phase3/skins/vector/images/watch_on.gif
Cannot display: file marked as a binary type.
svn:mime-type = application/octet-stream
Property changes on: trunk/phase3/skins/vector/images/watch_on.gif
___________________________________________________________________
Name: svn:mime-type
731733 + application/octet-stream
Index: trunk/phase3/skins/vector/main-ltr.css
——@@ -1042,6 +1042,7 @@
10431043 position: relative;
10441044 width: 100%;
10451045 }
 1046+
10461047 #mw-js-message {
10471048 font-size: 0.8em;
10481049 }
——@@ -1049,3 +1050,52 @@
10501051 line-height: 1.5em;
10511052 }
10521053
 1054+
 1055+
 1056+/* Babaco color scheme */
 1057+/* Still working on this. Needs incorporated above once it's closer to final */
 1058+body{
 1059+ background-color:#f9f9f9;
 1060+ color:#000000;
 1061+}
 1062+a{
 1063+ color:#003cb3;
 1064+}
 1065+a.new{
 1066+ color:#990000;
 1067+}
 1068+
 1069+a:visited,
 1070+div.vectorTabs li.selected a:visited div.vectorTabs li.selected a:visited span {
 1071+ color:#260e9c;
 1072+}
 1073+
 1074+html .thumbimage,
 1075+#toc, .toc, .mw-warning, div.thumbinner{
 1076+ border-color:#c0c0c0;
 1077+ background-color:#f0f0f0;
 1078+}
 1079+
 1080+h1, h2, h3, h4, h5, h6{
 1081+ border-color:#8d8d8d;
 1082+}
 1083+
 1084+
 1085+/* Watch/Unwatch Icon Styling */
 1086+#ca-unwatch,
 1087+#ca-watch{
 1088+ background-image:none;
 1089+ background-color:#ffffff;
 1090+}
 1091+#ca-unwatch a,
 1092+#ca-watch a{
 1093+ outline:none;
 1094+}
 1095+#ca-unwatch a img,
 1096+#ca-watch a img{
 1097+ padding-top:1.35em;
 1098+ display:block;
 1099+ font-size:0.8em;
 1100+ max-height:14px;
 1101+ max-width:14px;
 1102+}
\ No newline at end of file

Follow-up revisions

RevisionCommit summaryAuthorDate
r57006addressing feedback on the watch/unwatch javascript changes introduced in r56924adam02:45, 28 September 2009
r57012Fix for r56924: don't do any mouseover/mouseout effects while the spinner is ...catrope09:48, 28 September 2009
r57013Coding style cleanup for r56924; also copypaste main-ltr.css modifications to...catrope10:09, 28 September 2009
r57125Revert r56924 (new watch icon and color changes for Vector) and followups r57...catrope17:58, 30 September 2009

Comments

#Comment by Catrope (talk | contribs)   20:01, 25 September 2009 
+  	  wgAjaxWatch.imgBasePath = el1.firstChild.firstChild.src.replace(/\/skins\/vector\/images\/watch_(off|on).gif/, "");

This assumes that $wgStylePath == "/skins", which may not be the case (it's /skins-1.5 on Wikipedia, for instance). Use wgStylePath instead. 

+/* Babaco color scheme */
+/* Still working on this. Needs incorporated above once it's closer to final */

Did you intend to commit this?

#Comment by Adammiller~mediawikiwiki (talk | contribs)   20:36, 25 September 2009

Seems like I'd have to write wgStylePath to a javascript variable to access it there. Would it work if I instead left off the skins directory from the regex and did this? 

 	  wgAjaxWatch.imgBasePath = el1.firstChild.firstChild.src.replace(/\/vector\/images\/watch_(off|on).gif/, "");

The color scheme stuff I did intend to commit. Those styles are segregated right now because I keep getting changes to the color scheme from Hannes and I wanted to keep them at a easy to edit place. I'll roll my current colors into the proper rules and recommit.

#Comment by Catrope (talk | contribs)   20:38, 25 September 2009

If changing that regex still accomplishes what you want, it's probably the best solution.

No worries about the color scheme: it just looked likely to be an accident to me, so I thought I'd ask.

#Comment by Jack Phoenix (talk | contribs)   22:21, 25 September 2009

It should also be noted that the CSS/JS additions do not follow MediaWiki's coding style: we use tabs instead of spaces for indentation and a space after colons (in CSS) and brackets. See Manual:Coding conventions for details.

#Comment by Catrope (talk | contribs)   09:40, 26 September 2009

Also, this code generates HTML like: 

<li id="ca-watch">
  <a id="ca-watch" ...>
    <img .... />
  </a>
</li>

IDs have to be unique, so this snippet is invalid HTML. You probably don't need the ID on the a, because you can access it with a descendant selector in CSS anyway (like #ca-watch a).

The CSS you added always colors the watch tab white, giving it the 'active' look rather than the 'inactive' look. I don't personally like this, but let's hear what others think about it first.

#Comment by Catrope (talk | contribs)   09:43, 26 September 2009

Gah. The HTML I intended to quote was: 

<li id="ca-watch">
  <a id="ca-watch" ... >
    <img ... />
  </a>
</li>
#Comment by Adammiller~mediawikiwiki (talk | contribs)   02:48, 28 September 2009

Commit changes to address all of these issues in r57006.

#Comment by Catrope (talk | contribs)   09:57, 28 September 2009 
a:visited,
div.vectorTabs li.selected a:visited div.vectorTabs li.selected a:visited span {
	color:#260e9c;
}

Are you sure that's what you mean? div.vectorTabs li.selected a:visited div.vectorTabs li.selected a:visited span looks like a selector that doesn't match anything. Even if you mean div.vectorTabs li.selected a:visited, div.vectorTabs li.selected a:visited span (with a comma in between), the first part looks redundant (but it might not be; I haven't looked at the other CSS very thoroughly).

#Comment by Brion VIBBER (talk | contribs)   00:44, 30 September 2009

More cleanup done in r57030.

Status & tagging log