r85322 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85321‎ | r85322 | r85323 >
Date:11:46, 4 April 2011
Author:happy-melon
Status:reverted (Comments)
Tags:
Comment:
Add the revision-info and revision-info-current messages directly to the page, don't stick them in the content sub; this adds unwanted extra CSS styling which screws with the margin and font size.
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Article.php
——@@ -3805,7 +3805,7 @@
38063806 "\n\t\t\t\t<div id=\"mw-revision-nav\">" . $cdel . wfMsgExt( 'revision-nav', array( 'escapenoentities', 'parsemag', 'replaceafter' ),
38073807 $prevdiff, $prevlink, $lnk, $curdiff, $nextlink, $nextdiff ) . "</div>\n\t\t\t";
38083808
3809 - $wgOut->setSubtitle( $r );
 3809+ $wgOut->addHTML( $r );
38103810 }
38113811
38123812 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r90883Follow-up r85322: brion's right that some styling is desirable; so make it ce...happy-melon16:13, 27 June 2011
r91416Revert old page navigation back to a subtitle...hashar19:17, 4 July 2011
r91417Revert old page navigation back to a subtitle...hashar19:19, 4 July 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   23:47, 21 June 2011

That CSS styling was desirable -- now these bits are flush with article contents, which is super awful.

#Comment by MZMcBride (talk | contribs)   23:59, 21 June 2011

This.

I agree that sticking this in contentsub is a bit quirky (it feels hackish), but the lack of styling makes the whole block difficult to discover. I noticed this running trunk on a test wiki. I'm surprised this has sat for so long without others saying something, actually....

Perhaps it would make sense to move the whole thing to a <div> above the <h1>? I've long held that sticking notifications to the user (new messages bar included) in the article area is a bit silly. There should be some visual separation between what is article content and what is editing/user interface. Putting things between the title and the article text seems to disrupt this in a nasty way.

#Comment by Hashar (talk | contribs)   19:11, 4 July 2011

This change the setOldSubtitle() method behaviour. Instead of setting a title, it adds to the ouput. The method name (setOldSubtitle) is thus misleading.

Please consider MZMcBribe comment about adding the navigation bar above the <h1 id="FirstHeading"> thus outside of the article content and just below the js-message div. That will make more sens overall.


I am going to revert r85322 and its follow up r90883, that will let us advance a bit on 1.18. Please reapply a patch on trunk :)

#Comment by Hashar (talk | contribs)   19:19, 4 July 2011

reverted in both trunk and REL1_18. Please reapply with MZMcBribe proposal :b

Status & tagging log