r77121 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r77120‎ | r77121 | r77122 >
Date:23:52, 22 November 2010
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
Putting .config-page-list (sidebar nav) floating to the right as part of the page (instead of floating outside the page content). This takes away the dead white space on longer pages by making it grey instead of white (previously this made the page look very unorganized and off-grid in more narrow windows). This also fixes a layout bug in Safari where the tops didn't line up.
Modified paths:
  • /trunk/phase3/includes/installer/WebInstaller.php (modified) (history)
  • /trunk/phase3/skins/common/config.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/config.css
@@ -39,14 +39,15 @@
4040 float: right;
4141 width: 12em;
4242 border: 1px solid #aaa;
 43+ background: #fff;
4344 padding: 0.5em;
44 - margin: 0.5em;
 45+ /* 3em left margin to leave space between the list and the page-content */
 46+ margin: 0.5em 0.5em 0.5em 3.5em;
4547 }
4648
4749 .config-page {
48 - padding: 0.5em 2em 0.5em 2em;
49 - /* 15em right margin to leave space for 12em page list */
50 - margin: 0.5em 15em 0.5em 0.5em;
 50+ padding: 0.5em 0.5em 0.5em 2em;
 51+ margin: 0.5em 0.5em 0.5em 0.5em;
5152 background: #eee;
5253 }
5354
Index: trunk/phase3/includes/installer/WebInstaller.php
@@ -469,8 +469,9 @@
470470 * @param $currentPageName String
471471 */
472472 private function startPageWrapper( $currentPageName ) {
473 - $s = "<div class=\"config-page-wrapper\">\n" .
474 - "<div class=\"config-page-list\"><ul>\n";
 473+ $s = "<div class=\"config-page-wrapper\">\n";
 474+ $s .= "<div class=\"config-page\">\n";
 475+ $s .= "<div class=\"config-page-list\"><ul>\n";
475476 $lastHappy = -1;
476477
477478 foreach ( $this->pageSequence as $id => $pageName ) {
@@ -492,9 +493,8 @@
493494 $s .= $this->getPageListItem( $pageName, true, $currentPageName );
494495 }
495496
496 - $s .= "</ul></div>\n". // end list pane
497 - "<div class=\"config-page\">\n" .
498 - Html::element( 'h2', array(),
 497+ $s .= "</ul></div>\n"; // end list pane
 498+ $s .= Html::element( 'h2', array(),
499499 wfMsg( 'config-page-' . strtolower( $currentPageName ) ) );
500500
501501 $this->output->addHTMLNoFlush( $s );

Follow-up revisions

RevisionCommit summaryAuthorDate
r78946Follow-up r77121...krinkle01:41, 24 December 2010

Comments

#Comment by Happy-melon (talk | contribs)   14:39, 17 December 2010

You need another <br clear=both/> at the end of the config-pabe-wrapper div to stop the side box spilling out of the grey area on very short pages (eg the language select).

#Comment by Krinkle (talk | contribs)   03:35, 22 December 2010

Please dont use a line-break to clear the float. This causes unneeded whitespace. Also clear=both is invalid syntax, I believe you mean style=clear:both.

<div style="clear:both"></div>

or:

<div class="visualClear"></div>
#Comment by Happy-melon (talk | contribs)   11:08, 22 December 2010

The latter would indeed be the best option.

Status & tagging log