r69317 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69316‎ | r69317 | r69318 >
Date:22:31, 13 July 2010
Author:adam
Status:ok (Comments)
Tags:
Comment:
Epic bug fix. Changing the markup structure of Vectors tabs and related watch/unwatch code to address bug 23490
Modified paths:
  • /trunk/phase3/skins/Vector.php (modified) (history)
  • /trunk/phase3/skins/common/ajaxwatch.js (modified) (history)
  • /trunk/phase3/skins/vector/main-ltr.css (modified) (history)
  • /trunk/phase3/skins/vector/main-rtl.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/ajaxwatch.js
——@@ -73,7 +73,7 @@
7474 $links.each( function(){
7575 var $link = $j(this);
7676 $link
77 - .data( 'icon', $link.parent().hasClass( 'icon' ) )
 77+ .data( 'icon', $link.parents( 'li' ).hasClass( 'icon' ) )
7878 .data( 'action', $link.attr( 'href' ).match( /[\?\&]action=unwatch/i ) ? 'unwatch' : 'watch' );
7979 var title = $link.attr( 'href' ).match( /[\?\&]title=(.*?)&/i )[1];
8080 $link.data( 'target', decodeURIComponent( title ).replace( /_/g, ' ' ) );
——@@ -117,8 +117,8 @@
118118 $link.data( 'action', otheraction );
119119 wgAjaxWatch.setLinkText( $link, otheraction );
120120 $link.attr( 'href', $link.attr('href').replace( '/&action='+action+'/', '&action='+otheraction ) );
121 - if( $link.parent().attr('id') == 'ca-'+action ){
122 - $link.parent().attr( 'id', 'ca-'+otheraction );
 121+ if( $link.parents( 'li' ).attr('id') == 'ca-'+action ){
 122+ $link.parents( 'li' ).attr( 'id', 'ca-'+otheraction );
123123 }
124124 };
125125 return false;
Index: trunk/phase3/skins/Vector.php
——@@ -695,7 +695,7 @@
696696 <h5><?php $this->msg('namespaces') ?></h5>
697697 <ul<?php $this->html('userlangattributes') ?>>
698698 <?php foreach ($this->data['namespace_urls'] as $key => $link ): ?>
699 - <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>
 699+ <li <?php echo $link['attributes'] ?>><span><a href="<?php echo htmlspecialchars( $link['href'] ) ?>" <?php echo $link['key'] ?>><?php echo htmlspecialchars( $link['text'] ) ?></a></span></li>
700700 <?php endforeach; ?>
701701 </ul>
702702 </div>
——@@ -730,7 +730,7 @@
731731 <h5><?php $this->msg('views') ?></h5>
732732 <ul<?php $this->html('userlangattributes') ?>>
733733 <?php foreach ( $this->data['view_urls'] as $key => $link ): ?>
734 - <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>
 734+ <li<?php echo $link['attributes'] ?>><span><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'].'" />' : htmlspecialchars( $link['text'] ) ) ?></a></span></li>
735735 <?php endforeach; ?>
736736 </ul>
737737 </div>
Index: trunk/phase3/skins/vector/main-ltr.css
——@@ -158,19 +158,18 @@
159159 /* OVERRIDDEN BY COMPLIANT BROWSERS */
160160 div.vectorTabs li a {
161161 display: inline-block;
162 - height: 2.5em;
163 - padding-left: 0.4em;
164 - padding-right: 0.4em;
 162+ height: 1.9em;
 163+ padding-left: 0.5em;
 164+ padding-right: 0.5em;
165165 background-image: url(images/tab-break.png?1);
166166 background-position: bottom right;
167167 background-repeat: no-repeat;
168168 }
169 - div.vectorTabs li a,
170 - div.vectorTabs li a span {
 169+ div.vectorTabs li a{
171170 color: #0645ad;
172171 cursor: pointer;
173172 }
174 - div.vectorTabs li a span {
 173+ div.vectorTabs li a {
175174 font-size: 0.8em;
176175 }
177176 /* IGNORED BY IE6 */
——@@ -178,27 +177,23 @@
179178 display: block;
180179 }
181180 /* OVERRIDDEN BY COMPLIANT BROWSERS */
182 - div.vectorTabs a span {
 181+ div.vectorTabs span a {
183182 display: inline-block;
184183 padding-top: 1.25em;
185184 }
186185 /* IGNORED BY IE6 */
187186 /* @noflip */
188 - div.vectorTabs a > span {
 187+ div.vectorTabs span > a {
189188 float: left;
190189 display: block;
191190 }
192191 div.vectorTabs li.selected a,
193 - div.vectorTabs li.selected a span,
194 - div.vectorTabs li.selected a:visited
195 - div.vectorTabs li.selected a:visited span {
 192+ div.vectorTabs li.selected a:visited{
196193 color: #333333;
197194 text-decoration: none;
198195 }
199196 div.vectorTabs li.new a,
200 - div.vectorTabs li.new a span,
201 - div.vectorTabs li.new a:visited,
202 - div.vectorTabs li.new a:visited span {
 197+ div.vectorTabs li.new a:visited{
203198 color: #a55858;
204199 }
205200 /* Variants and Actions */
——@@ -1104,7 +1099,8 @@
11051100 outline: none;
11061101 display: block;
11071102 width: 26px;
1108 - height: 2.5em;
 1103+ height: 3.1em;
 1104+ text-indent: -9999px;
11091105 }
11101106 #ca-unwatch.icon a {
11111107 background-image: url(images/watch-icons.png?1);
Index: trunk/phase3/skins/vector/main-rtl.css
——@@ -158,19 +158,18 @@
159159 /* OVERRIDDEN BY COMPLIANT BROWSERS */
160160 div.vectorTabs li a {
161161 display: inline-block;
162 - height: 2.5em;
163 - padding-right: 0.4em;
164 - padding-left: 0.4em;
 162+ height: 1.9em;
 163+ padding-right: 0.5em;
 164+ padding-left: 0.5em;
165165 background-image: url(images/tab-break.png?1);
166166 background-position: bottom left;
167167 background-repeat: no-repeat;
168168 }
169 - div.vectorTabs li a,
170 - div.vectorTabs li a span {
 169+ div.vectorTabs li a{
171170 color: #0645ad;
172171 cursor: pointer;
173172 }
174 - div.vectorTabs li a span {
 173+ div.vectorTabs li a {
175174 font-size: 0.8em;
176175 }
177176 /* IGNORED BY IE6 */
——@@ -178,27 +177,23 @@
179178 display: block;
180179 }
181180 /* OVERRIDDEN BY COMPLIANT BROWSERS */
182 - div.vectorTabs a span {
 181+ div.vectorTabs span a {
183182 display: inline-block;
184183 padding-top: 1.25em;
185184 }
186185 /* IGNORED BY IE6 */
187186 /* @noflip */
188 - div.vectorTabs a > span {
 187+ div.vectorTabs span > a {
189188 float: left;
190189 display: block;
191190 }
192191 div.vectorTabs li.selected a,
193 - div.vectorTabs li.selected a span,
194 - div.vectorTabs li.selected a:visited
195 - div.vectorTabs li.selected a:visited span {
 192+ div.vectorTabs li.selected a:visited{
196193 color: #333333;
197194 text-decoration: none;
198195 }
199196 div.vectorTabs li.new a,
200 - div.vectorTabs li.new a span,
201 - div.vectorTabs li.new a:visited,
202 - div.vectorTabs li.new a:visited span {
 197+ div.vectorTabs li.new a:visited{
203198 color: #a55858;
204199 }
205200 /* Variants and Actions */
——@@ -1104,7 +1099,8 @@
11051100 outline: none;
11061101 display: block;
11071102 width: 26px;
1108 - height: 2.5em;
 1103+ height: 3.1em;
 1104+ text-indent: -9999px;
11091105 }
11101106 #ca-unwatch.icon a {
11111107 background-image: url(images/watch-icons.png?1);

Follow-up revisions

RevisionCommit summaryAuthorDate
r69329style version bump for r69317adam15:06, 14 July 2010
r69649This improves on the CSS hack used in r69317 - which worked fine but in some ...tparscal23:47, 20 July 2010

Comments

#Comment by Platonides (talk | contribs)   23:02, 13 July 2010

Nice!

But is that text-indent: -9999px on #ca-[un]watch.icon really needed? It's a hackish way of hiding the text "watch".

#Comment by Adammiller~mediawikiwiki (talk | contribs)   15:03, 14 July 2010

It's necessary to hide the next. I doubt the way I'm doing it is the only way to achieve that, but it (mostly) works.

I'm looking into other solutions. Any ideas are welcome.

This made me notice that the main logo is handled in a similar, yet worse way. It's an a tag with the logo image loaded as the background, but instead of using css to hide the text, the tag has no text at all in it. If a user has css disabled, they see nothing. If a user has images disabled, they see nothing.

#Comment by Simetrical (talk | contribs)   22:43, 14 July 2010

This is fine. The logo is just for decoration, so the appropriate alt text is nothing. The only other thing you might put there is the site name, but that's already repeated in several places on the page, so the empty string is fine.

#Comment by TheDJ (talk | contribs)   23:15, 20 July 2010

wasn't there an issue with such indents and rtl displaying ? We might wanna check for "ultra wide pages" in a rtl setup.

#Comment by Nikerabbit (talk | contribs)   06:52, 14 July 2010

Forgot to update style version?

#Comment by Adammiller~mediawikiwiki (talk | contribs)   15:07, 14 July 2010

I certainly did. Fixed in r69329.

#Comment by Trevor Parscal (WMF) (talk | contribs)   23:11, 20 July 2010

Clever CSS hack :)

#Comment by Trevor Parscal (WMF) (talk | contribs)   23:51, 20 July 2010

I did you one better in r69649 by using the following properties..

height: 0; overflow: hidden; padding-top: 3.1em;

Notice the 3.1em came from what used to be the height property.

Status & tagging log