r56045 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r56044‎ | r56045 | r56046 >
Date:17:32, 8 September 2009
Author:vyznev
Status:ok (Comments)
Tags:
Comment:
Improved compatibility between the Vector skin and addPortletLink() from wikibits.js:
* Empty portlets are now present but hidden; adding an element to a portlet unhides it.
* Drop-down menu portlets now have their own left border, so they'll look right even if there's no other portlet to the left of them. (If there is, the borders should overlap.)
* addPortletLink() now wraps its labels in <span> tags to be compatible with the Vector CSS for the "namespaces" and "views" portlets (bug 19531).
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/skins/Vector.php (modified) (history)
  • /trunk/phase3/skins/common/wikibits.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/wikibits.js
——@@ -287,13 +287,19 @@
288288 * @return Node -- the DOM node of the new item (an LI element) or null
289289 */
290290 function addPortletLink(portlet, href, text, id, tooltip, accesskey, nextnode) {
291 - var node = document.getElementById(portlet);
 291+ var root = document.getElementById(portlet);
 292+ if ( !root ) return null;
 293+ var node = root.getElementsByTagName( "ul" )[0];
292294 if ( !node ) return null;
293 - node = node.getElementsByTagName( "ul" )[0];
294 - if ( !node ) return null;
295295
 296+ // unhide portlet if it was hidden before
 297+ root.className = root.className.replace( /(^| )emptyPortlet( |$)/, "$2" );
 298+
 299+ var span = document.createElement( "span" );
 300+ span.appendChild( document.createTextNode( text ) );
 301+
296302 var link = document.createElement( "a" );
297 - link.appendChild( document.createTextNode( text ) );
 303+ link.appendChild( span );
298304 link.href = href;
299305
300306 var item = document.createElement( "li" );
Index: trunk/phase3/skins/Vector.php
——@@ -691,9 +691,8 @@
692692 echo "\n<!-- {$name} -->\n";
693693 switch ( $element ) {
694694 case 'NAMESPACES':
695 - if ( count( $this->data[ 'namespace_urls' ] ) > 0 ) {
696695 ?>
697 -<div id="namespaces" class="vectorTabs">
 696+<div id="namespaces" class="vectorTabs<?php if ( count( $this->data['namespace_urls'] ) == 0 ) echo ' emptyPortlet'; ?>">
698697 <h5><?php $this->msg('namespaces') ?></h5>
699698 <ul <?php $this->html('userlangattributes') ?>>
700699 <?php foreach ($this->data['namespace_urls'] as $key => $link ): ?>
——@@ -702,12 +701,10 @@
703702 </ul>
704703 </div>
705704 <?php
706 - }
707705 break;
708706 case 'VARIANTS':
709 - if ( count( $this->data[ 'variant_urls' ] ) > 0 ) {
710707 ?>
711 -<div id="variants" class="vectorMenu">
 708+<div id="variants" class="vectorMenu<?php if ( count( $this->data['variant_urls'] ) == 0 ) echo ' emptyPortlet'; ?>">
712709 <h5><span><?php $this->msg('variants') ?></span><a href="#"></a></h5>
713710 <div class="menu">
714711 <ul <?php $this->html('userlangattributes') ?>>
——@@ -718,12 +715,10 @@
719716 </div>
720717 </div>
721718 <?php
722 - }
723719 break;
724720 case 'VIEWS':
725 - if ( count( $this->data[ 'view_urls' ] ) > 0 ) {
726721 ?>
727 -<div id="views" class="vectorTabs">
 722+<div id="views" class="vectorTabs<?php if ( count( $this->data['view_urls'] ) == 0 ) echo ' emptyPortlet'; ?>">
728723 <h5><?php $this->msg('views') ?></h5>
729724 <ul <?php $this->html('userlangattributes') ?>>
730725 <?php foreach ($this->data['view_urls'] as $key => $link ): ?>
——@@ -732,12 +727,10 @@
733728 </ul>
734729 </div>
735730 <?php
736 - }
737731 break;
738732 case 'ACTIONS':
739 - if ( count( $this->data[ 'action_urls' ] ) > 0 ) {
740733 ?>
741 -<div id="p-cactions" class="vectorMenu">
 734+<div id="p-cactions" class="vectorMenu<?php if ( count( $this->data['action_urls'] ) == 0 ) echo ' emptyPortlet'; ?>">
742735 <h5><span><?php $this->msg('actions') ?></span><a href="#"></a></h5>
743736 <div class="menu">
744737 <ul <?php $this->html('userlangattributes') ?>>
——@@ -748,12 +741,10 @@
749742 </div>
750743 </div>
751744 <?php
752 - }
753745 break;
754746 case 'PERSONAL':
755 - if ( count( $this->data['personal_urls'] ) > 0 ) {
756747 ?>
757 -<div id="p-personal">
 748+<div id="p-personal" class="<?php if ( count( $this->data['personal_urls'] ) == 0 ) echo ' emptyPortlet'; ?>">
758749 <h5><?php $this->msg('personaltools') ?></h5>
759750 <ul <?php $this->html('userlangattributes') ?>>
760751 <?php foreach($this->data['personal_urls'] as $key => $item): ?>
——@@ -762,7 +753,6 @@
763754 </ul>
764755 </div>
765756 <?php
766 - }
767757 break;
768758 case 'SEARCH':
769759 ?>
Index: trunk/phase3/skins/vector/main-ltr.css
——@@ -62,6 +62,10 @@
6363 margin: 0;
6464 padding: 0;
6565 }
 66+ /* Hide empty portlets */
 67+ div.emptyPortlet {
 68+ display: none;
 69+ }
6670 /* Personal */
6771 #p-personal {
6872 position: absolute;
——@@ -204,8 +208,13 @@
205209 direction: rtl;
206210 }
207211 /* @noflip */
208 - div.vectorMenu h5 {
 212+ #head div.vectorMenu h5 {
209213 float: left;
 214+ background-image: url(images/tab-break.png);
 215+ background-position: bottom left;
 216+ background-repeat: no-repeat;
 217+ padding-left: 1px;
 218+ margin-left: -1px;
210219 }
211220 /* OVERRIDDEN BY COMPLIANT BROWSERS */
212221 div.vectorMenu h5 a {
Index: trunk/phase3/skins/vector/main-rtl.css
——@@ -62,6 +62,10 @@
6363 margin: 0;
6464 padding: 0;
6565 }
 66+ /* Hide empty portlets */
 67+ div.emptyPortlet {
 68+ display: none;
 69+ }
6670 /* Personal */
6771 #p-personal {
6872 position: absolute;
——@@ -204,8 +208,13 @@
205209 direction: rtl;
206210 }
207211 /* @noflip */
208 - div.vectorMenu h5 {
 212+ #head div.vectorMenu h5 {
209213 float: left;
 214+ background-image: url(images/tab-break.png);
 215+ background-position: bottom left;
 216+ background-repeat: no-repeat;
 217+ padding-left: 1px;
 218+ margin-left: -1px;
210219 }
211220 /* OVERRIDDEN BY COMPLIANT BROWSERS */
212221 div.vectorMenu h5 a {
Index: trunk/phase3/RELEASE-NOTES
——@@ -474,6 +474,10 @@
475475 any change, in addition to onkeyup.
476476 * Don't link to "edit this page" on MediaWiki:Noarticletext if user is not allowed
477477 to create page. Done via new message MediaWiki:Noarticletext-nopermission
 478+* Improved compatibility between the Vector skin and addPortletLink() from wikibits.js:
 479+ empty portlets are now present but hidden, adding an element to a portlet unhides it
 480+* (bug 19531) addPortletLink() now wraps inserted labels in a <span> element to be
 481+ compatible with the CSS for the Vector skin
478482
479483 == API changes in 1.16 ==
480484

Comments

#Comment by Brion VIBBER (talk | contribs)   19:08, 8 September 2009

Looks ok per Trevor.

#Comment by Trevor Parscal (WMF) (talk | contribs)   19:15, 8 September 2009

This looks ok, but why did we add #head to the div.vectorMenu h5 rule? In firebug on FF, I see no diff - but perhaps there was a good reason here.

#Comment by Ilmari Karonen (talk | contribs)   21:32, 8 September 2009

The #head is needed to make this rule more specific than the earlier rule: 

#head h5 {
	margin: 0;
	padding: 0;
}

Without it, the margin-left and padding-left settings I added for div.vectorMenu h5 get overridden by the more specific #head h5 rule, causing the borders not to overlap as they should.

#Comment by Trevor Parscal (WMF) (talk | contribs)   19:16, 10 September 2009

Sounds reasonable to me...

Status & tagging log