r99307 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r99306‎ | r99307 | r99308 >
Date:12:59, 8 October 2011
Author:hartman
Status:reverted (Comments)
Tags:
Comment:
Add !important to the tablesorter indicator.

Otherwise it is too easy to do <th style="background:red"> and kill the entire indicator.
Fixes bug 31196 and follow up to r98665
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.tablesorter.css (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.tablesorter.css
——@@ -1,17 +1,17 @@
22 /* Table Sorting */
33 table.jquery-tablesorter th.headerSort {
44 /* @embed */
5 - background-image: url(images/sort_both.gif);
 5+ background-image: url(images/sort_both.gif) !important;
66 cursor: pointer;
7 - background-repeat: no-repeat;
8 - background-position: center right;
9 - padding-right: 21px;
 7+ background-repeat: no-repeat !important;
 8+ background-position: center right !important;
 9+ padding-right: 21px !important;
1010 }
1111 table.jquery-tablesorter th.headerSortUp {
1212 /* @embed */
13 - background-image: url(images/sort_up.gif);
 13+ background-image: url(images/sort_up.gif) !important;
1414 }
1515 table.jquery-tablesorter th.headerSortDown {
1616 /* @embed */
17 - background-image: url(images/sort_down.gif);
 17+ background-image: url(images/sort_down.gif) !important;
1818 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r99309Follow up to r99307, with some inline comment documentation on why these !imp...hartman13:07, 8 October 2011
r102452Revert r99307 and its friend r99309, improper use of !important per CRcatrope21:38, 8 November 2011
r103155Swap spaces to tabs (creates irritating merge conflicts for r99307)reedy12:45, 15 November 2011
r103156REL1_18 Merge r99307 cleanly after r103155reedy12:47, 15 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r98665Add the class jquery-tablesorter to all tables that are made sortable....hartman11:08, 2 October 2011

Comments

#Comment by Krinkle (talk | contribs)   23:34, 9 October 2011

Recommending not to !important the padding-right declaration.

Consider: 

--- MediaWiki:Common.css

table.unicorn-tables th {
  padding: 50px 100px;
  background-color: pink;
}

or anything increasing the padding.

#Comment by TheDJ (talk | contribs)   14:17, 10 October 2011

In that case i suggest we back out tablesorter all together and restore the old functionality with a seperate sortindicator element.

#Comment by Hashar (talk | contribs)   13:35, 8 November 2011

Why should we revert the table sorter all together? That does not make any sense to me.

The is merely a workaround in case someone use the CSS short-hand "background" instead of "background-color". I am not sure it is worth it but it does not seem to cause any trouble.

But it also means that users will not be able to customize the background images now :-D

#Comment by Krinkle (talk | contribs)   18:57, 8 November 2011

Still, there is no need for !important here.

I believe there is no excuse of !important in core stylesheets at any time (except maybe for vendor/IE-hacks). There are ways around it, and in this case the work-around is already in place. The selector here is more specific than the selector that the local wikis have.

Example: 

-- mw core

table.jquery-tablesorter th.headerSort {
  color: green;
}


-- local wiki 

table.sortable th {
  color: red;
}


-- html

<table class="sortable jquery-tablesorter">
  <tr>
    <th> This text is green </th>
  </tr>
</table>

Please remove the !important.

#Comment by RobLa-WMF (talk | contribs)   22:29, 14 November 2011

Tagging 1.18, since we don't have a permafix and this fix seems to be better than leaving it broken.

#Comment by Catrope (talk | contribs)   09:43, 15 November 2011

Untagging 1.18, this revision has been reverted (!) per Krinkle's review.

#Comment by Catrope (talk | contribs)   09:56, 15 November 2011

...and retagging after reading the discussion on bug 31755. Would have been nice if that had been mentioned when it was tagged for 1.18 in the first place.

Status & tagging log