r92054 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r92053‎ | r92054 | r92055 >
Date:10:41, 13 July 2011
Author:diebuche
Status:ok (Comments)
Tags:post1.18deploy 
Comment:
Render category links as an HTML list. Bug 12261. Based on patch by Thana & Bergi. It's removing the textual pipe separator, wrapping the links inside li elements and adding a left 1px border as separator. This makes it much easier to manipulate the list via JS or CSS and is also semantically correct
Modified paths:
  • /trunk/phase3/includes/Skin.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messageTypes.inc (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)
  • /trunk/phase3/skins/CologneBlue.php (modified) (history)
  • /trunk/phase3/skins/Nostalgia.php (modified) (history)
  • /trunk/phase3/skins/common/shared.css (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
@@ -3192,7 +3192,6 @@
31933193 'confirm-purge-bottom',
31943194 ),
31953195 'separators' => array(
3196 - 'catseparator',
31973196 'semicolon-separator',
31983197 'comma-separator',
31993198 'colon-separator',
Index: trunk/phase3/maintenance/language/messageTypes.inc
@@ -378,7 +378,6 @@
379379 'hebrew-calendar-m12-gen',
380380 'version-api',
381381 'version-svn-revision',
382 - 'catseparator',
383382 'semicolon-separator',
384383 'comma-separator',
385384 'colon-separator',
Index: trunk/phase3/skins/CologneBlue.php
@@ -77,7 +77,7 @@
7878
7979 $s .= '<font size="-1"><span id="langlinks">';
8080 $s .= str_replace( '<br />', '', $this->otherLanguages() );
81 - $cat = $this->getSkin()->getCategoryLinks();
 81+ $cat = '<div id="catlinks" class="catlinks">' . $this->getSkin()->getCategoryLinks() . '</div>';
8282 if( $cat ) {
8383 $s .= "<br />$cat\n";
8484 }
Index: trunk/phase3/skins/common/shared.css
@@ -179,6 +179,27 @@
180180 .magnify { float: right; }
181181
182182 /**
 183+ * Categories
 184+ */
 185+#catlinks ul, #catlinks li {
 186+ display:inline;
 187+ margin: 0px;
 188+ list-style:none;
 189+ list-style-type:none;
 190+ list-style-image:none;
 191+}
 192+
 193+#catlinks li {
 194+ padding: 0 .7em;
 195+ border-left: 1px solid #AAA;
 196+}
 197+
 198+#catlinks li:first-child {
 199+ padding-left: .4em;
 200+ border-left: none;
 201+}
 202+
 203+/**
183204 * Hidden categories
184205 */
185206 .mw-hidden-cats-hidden { display: none; }
Index: trunk/phase3/skins/Nostalgia.php
@@ -54,7 +54,7 @@
5555 $s .= '<br />' . $ol;
5656 }
5757
58 - $cat = $this->getSkin()->getCategoryLinks();
 58+ $cat = '<div id="catlinks" class="catlinks">' . $this->getSkin()->getCategoryLinks() . '</div>';
5959 if( $cat ) {
6060 $s .= '<br />' . $cat;
6161 }
Index: trunk/phase3/includes/Skin.php
@@ -530,26 +530,23 @@
531531 return '';
532532 }
533533
534 - # Separator
535 - $sep = wfMsgExt( 'catseparator', array( 'parsemag', 'escapenoentities' ) );
536 -
537534 // Use Unicode bidi embedding override characters,
538535 // to make sure links don't smash each other up in ugly ways.
539536 $dir = $wgContLang->getDir();
540 - $embed = "<span dir='$dir'>";
541 - $pop = '</span>';
 537+ $embed = "<li dir='$dir'>";
 538+ $pop = "</li>";
542539
543540 $allCats = $out->getCategoryLinks();
544541 $s = '';
545542 $colon = wfMsgExt( 'colon-separator', 'escapenoentities' );
546543
547544 if ( !empty( $allCats['normal'] ) ) {
548 - $t = $embed . implode( "{$pop} {$sep} {$embed}" , $allCats['normal'] ) . $pop;
 545+ $t = $embed . implode( "{$pop} {$embed}" , $allCats['normal'] ) . $pop;
549546
550547 $msg = wfMsgExt( 'pagecategories', array( 'parsemag', 'escapenoentities' ), count( $allCats['normal'] ) );
551548 $s .= '<div id="mw-normal-catlinks">' .
552549 Linker::link( Title::newFromText( wfMsgForContent( 'pagecategorieslink' ) ), $msg )
553 - . $colon . $t . '</div>';
 550+ . $colon . '<ul>' . $t . '</ul>' . '</div>';
554551 }
555552
556553 # Hidden categories
@@ -564,7 +561,7 @@
565562
566563 $s .= "<div id=\"mw-hidden-catlinks\" class=\"$class\">" .
567564 wfMsgExt( 'hidden-categories', array( 'parsemag', 'escapenoentities' ), count( $allCats['hidden'] ) ) .
568 - $colon . $embed . implode( "$pop $sep $embed", $allCats['hidden'] ) . $pop .
 565+ $colon . '<ul>' . $embed . implode( "{$pop} {$embed}" , $allCats['hidden'] ) . $pop . '</ul>' .
569566 '</div>';
570567 }
571568
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -510,7 +510,6 @@
511511 'accesskey-t-specialpages',
512512 'accesskey-t-whatlinkshere',
513513 'anonnotice',
514 - 'catseparator',
515514 'colon-separator',
516515 'currentevents',
517516 'currentevents-url',
@@ -4293,7 +4292,6 @@
42944293 'confirm-unwatch-top' => 'Remove this page to your watchlist?',
42954294
42964295 # Separators for various lists, etc.
4297 -'catseparator' => '|', # only translate this message to other languages if you have to change it
42984296 'semicolon-separator' => ';&#32;', # only translate this message to other languages if you have to change it
42994297 'comma-separator' => ',&#32;', # only translate this message to other languages if you have to change it
43004298 'colon-separator' => ':&#32;', # only translate this message to other languages if you have to change it

Follow-up revisions

RevisionCommit summaryAuthorDate
r92062Follow-up r92054: dir attributes are no longer needed (displays it incorrectl...robin13:33, 13 July 2011
r92153r92054 : Remove leftover spacediebuche11:42, 14 July 2011
r92245r92054 : Fix wordwrap by using inline-block. This has the nice sideeffect of ...diebuche15:06, 15 July 2011
r93062r92054: Some IE fixesdiebuche16:51, 25 July 2011
r96390Followup r92054, these should use css classes not ids.dantman00:37, 7 September 2011
r96775Adress fixme on r92054: Style printdiebuche09:16, 11 September 2011
r97381Adding padding:0px to '.catlinks ul' (Follow-up r92054). Browsers sometimes h...krinkle18:01, 17 September 2011
r97382Use Skin->getCategories() instead of Skin->getCategoryLinks() and manually wr...krinkle18:03, 17 September 2011

Comments

#Comment by Fomafix (talk | contribs)   10:57, 14 July 2011

The space in "{$pop} {$embed}" must be removed, because it is rendered as </li> <li>. This space is visible and shifts the separator border out of the middle between the two words.

#Comment by DieBuche (talk | contribs)   11:42, 14 July 2011

Well spotted. Fixed in r92153

#Comment by Fomafix (talk | contribs)   12:35, 14 July 2011

The style definition for printing is missing.

#Comment by DieBuche (talk | contribs)   12:54, 15 July 2011

I'm not sure: Should categories even be printed? We strip out a lot of other stuff and they're mostly useful for the discovery of articles, but not so on paper.

#Comment by P858snake (talk | contribs)   12:56, 15 July 2011

I don't believe we print them currently.

#Comment by DieBuche (talk | contribs)   14:24, 15 July 2011

Just tried: Currently we do.

#Comment by TheDJ (talk | contribs)   16:52, 25 July 2011

We should inform Lupo, prime maintainer of HotCat

#Comment by DieBuche (talk | contribs)   16:57, 25 July 2011

Just did so.

#Comment by Krinkle (talk | contribs)   20:01, 24 August 2011

This revision introduces an issue.

The <div id="catlinks"> are outputted form within the skin-implementations (ie. ColongneBlue.php), but that ID is styled from common.css, please centralize that output so that the ID-naming is the same acros skins (or move the styling to skin's stylesheet).

#Comment by Krinkle (talk | contribs)   20:05, 24 August 2011

Hm.. seems it is already centralized. Skin.php defines a method Skin->getCategories() which wraps <div id="catlinks"> around getCategoryLinks() , perhaps use that instead ?

#Comment by Dantman (talk | contribs)   02:30, 5 September 2011

catlinks / getCategories needs to die horrible death... Style of things like these should be up to the skin that's actually styling the skin. This css should be in the individual skin stylesheets (like the rest of usermessage, header, toc, siteSub, etc...) and should be moved to commonInterface.css in 1.19. It should also use the css class, not the id.

#Comment by Catrope (talk | contribs)   13:03, 9 September 2011

Is there still a good reason for this to be marked fixme?

#Comment by Dantman (talk | contribs)   13:09, 9 September 2011

Print styles are still broken by this.

#Comment by DieBuche (talk | contribs)   09:16, 11 September 2011
#Comment by Krinkle (talk | contribs)   16:39, 17 September 2011

Hm.. seems it is already centralized. Skin.php defines a method Skin->getCategories() which wraps around getCategoryLinks() , perhaps use that instead ?

Any thoughts on that ? Is there a reason for not using that ?

#Comment by Dantman (talk | contribs)   16:42, 17 September 2011

The skins being legacy skins and being to lazy sounds like a good enough reason to me... I want to just get rid of those skins.

#Comment by Krinkle (talk | contribs)   18:04, 17 September 2011

That's not a good reason to me. They are in trunk, so either remove it or maintain it. I've done it myself in r97382.

There was also an issue with padding (see http://i.imgur.com/JWJvp.png). Fixed in r97381.

Marking OK, rest looks good.

#Comment by Dantman (talk | contribs)   18:05, 17 September 2011
) I hope I can do that by 1.20... I'm waiting for stats.
#Comment by P858snake (talk | contribs)   07:33, 5 October 2011

Release Notes?

#Comment by P858snake (talk | contribs)   07:46, 5 October 2011

This is tagged "post1.18deploy" but appears to be apart of the 1.18 deploy, someone feel like detagging?

#Comment by Bawolff (talk | contribs)   04:27, 9 October 2011

Causes bug 31551.

Status & tagging log