r85022 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85021‎ | r85022 | r85023 >
Date:17:37, 30 March 2011
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
(bug 19942) remove deprecated HTML attributes (cellpadding, align, valign) from TablePager.
Modified paths:
  • /trunk/phase3/includes/Pager.php (modified) (history)
  • /trunk/phase3/skins/common/shared.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/shared.css
——@@ -379,7 +379,15 @@
380380 .TablePager {
381381 min-width: 80%;
382382 border-collapse: collapse;
 383+ margin: 0 auto;
383384 }
 385+.TablePager_nav {
 386+ margin: 0 auto;
 387+}
 388+.TablePager_nav td {
 389+ padding: 3px;
 390+ text-align: center;
 391+}
384392 .TablePager_nav a {
385393 text-decoration: none;
386394 }
Index: trunk/phase3/includes/Pager.php
——@@ -815,7 +815,7 @@
816816 $tableClass = htmlspecialchars( $this->getTableClass() );
817817 $sortClass = htmlspecialchars( $this->getSortHeaderClass() );
818818
819 - $s = "<table border='1' class=\"$tableClass\"><thead><tr>\n";
 819+ $s = "<table style='border:1;' class=\"$tableClass\"><thead><tr>\n";
820820 $fields = $this->getFieldNames();
821821
822822 # Make table header
——@@ -982,10 +982,10 @@
983983 $links = $this->getPagingLinks( $linkTexts, $disabledTexts );
984984
985985 $navClass = htmlspecialchars( $this->getNavClass() );
986 - $s = "<table class=\"$navClass\" align=\"center\" cellpadding=\"3\"><tr>\n";
987 - $cellAttrs = 'valign="top" align="center" width="' . 100 / count( $links ) . '%"';
 986+ $s = "<table class=\"$navClass\"><tr>\n";
 987+ $width = 100 / count( $links ) . '%';
988988 foreach ( $labels as $type => $label ) {
989 - $s .= "<td $cellAttrs>{$links[$type]}</td>\n";
 989+ $s .= "<td style='width:$width;'>{$links[$type]}</td>\n";
990990 }
991991 $s .= "</tr></table>\n";
992992 return $s;

Follow-up revisions

RevisionCommit summaryAuthorDate
r89618Partial revert of r85022. Removing added auto-margin that changed behaviour.krinkle00:08, 7 June 2011

Comments

#Comment by Bryan (talk | contribs)   20:00, 12 April 2011

This makes TablePagers jump to the center of the parent, which is ugly in most uses. Was this intended?

#Comment by Happy-melon (talk | contribs)   20:01, 12 April 2011

Yes, centering was intended (that's the margin: 0 auto). Why do you think this is messy?

#Comment by Bryan (talk | contribs)   20:05, 12 April 2011

Because the pager is used mostly on special pages where there is an input form above it which is left aligned.


Also, when you are making style changes that change the appearance, please add that to your commit message; it is hard to guess your intention without that.

#Comment by Krinkle (talk | contribs)   20:11, 12 April 2011

The style didn't change afaik (if it did, something may be overriding it)

Look: 

- ... align=\"center\" ..
+ margin: 0 auto;

Browsers do this internally: 

table[align=center]{
 margin-left: auto;
 margin-right: auto;
}
#Comment by Bryan (talk | contribs)   20:15, 12 April 2011

That one corresponds to 

+.TablePager_nav {
+	margin: 0 auto;
+}

which is the navigation. The main table itself never had align=center.

#Comment by Krinkle (talk | contribs)   20:20, 12 April 2011

I see, nevermind in that case :)

#Comment by Happy-melon (talk | contribs)   20:11, 12 April 2011

Forms are (or should be) 100% width; IMO having a left-aligned block below that is more ugly, not less, especially when the paging arrows are centered. Most TablePagers are (at least) 100% width anyway.

#Comment by Happy-melon (talk | contribs)   16:22, 19 April 2011

I don't think there's anything to "fix" in what's been raised above; it might be disagreed on and/or reverted, but (that element of it) is not 'broken'.

#Comment by Krinkle (talk | contribs)   16:18, 2 June 2011

This is currently making the Special:Code/myrepo of Extensions:CodeReview look weirder than purple ponies wearing yellow suits.

#Comment by Krinkle (talk | contribs)   16:19, 2 June 2011

In trunk ^

#Comment by Happy-melon (talk | contribs)   18:23, 2 June 2011

I don't see any wierdness, let alone purple ponies, in my repo. What exact hallucinogenic effects are you experiencing? :P

#Comment by Krinkle (talk | contribs)   18:23, 2 June 2011

More presicely, as seen in Safari and Chrome, the table does not adhere 80% min-widtha and is centered, which looks weird since it should be wider I guess. And the layout of code review kinda assumes that there aren't any centered elements on the page (other than "Pager nav").

Compare:

http://i.imgur.com/dzkRo.png

http://i.imgur.com/9Luyy.png

#Comment by Krinkle (talk | contribs)   00:09, 7 June 2011

In r89618 I've reverted the change in this commit that was unrelated to the commit summary and made the TablePager element centered. Whether this was an improvement is a bigger decision.

See also this one http://i.imgur.com/Q0wCF.png where the Sign-offs and Follow-up revision tables are centered, wheareas that was not originally the case.

If you think the styles themself should change, please create an enhancement bug.

Status & tagging log