Improve the look and the usability of standard tables

I like not wrapping the asc/desc control alone. It should be tied to the last word in the header. I don't like not wrapping the header at all, though. When the header is sufficiently long, and the content in the column is sufficiently short, I would much rather have it wrap...especially if I am trying to fit a lot of columns in a table. I am undecided about always forcing tables to be 100% width. I can think of cases where I may not want that. Finally, I am more or less indifferent with regards to the cell borders. They don't bother me, and actually help make distinctions between units of data (cells).

Couldn't this all just be handled in the theme or CSS, though? Why patch core? The only thing I see that may be worth adding to core is not wrapping the asc/desc control by itself. That may be a small usability issue. Everything else regarding the look, though, seems to be subjective.

My (personal, scientifically unproven) opinion is that I like the second one.

I committed this patch to HEAD.

The 'width: 100%;' makes wide tables look better, but makes narrow tables look weird (eg. admin/user/role). We need to fine-tune (or revert) that, which is why I set the status of this report to 'active'.

The proposed improvements look good. I like version #1 best as it is easier for the eye (less intrusive). It makes the tables look really nice.

For the pretty tables to work, we'd have to extend the emitted HTML code. Suggestions for how this HTML code should look like so we can do both version #1 and version #2 of the proposed tables?

attached patch adds a class attribute called 'cell-highlight' to all cells within the actively sorted column. This class is not new - its the one we currently use in the table header. any themes currently defining this css class will automatically enjoy full column highlighting (e.g. phptemplate). Other themes may want to take advantage as well.

1. Why do we do is_array($header) in if (is_array($header) && $i === $ts['index']) { when $header does not appear to be used further down?
2. Is cell-highlight a good name? As a theme designer I wouldn't realize it is used to denote a cell that belongs to the active column. Maybe <tr class="active"> is more semantic (<tr>, not <td>)?

new patch which
- more secure tablesort SQL
- changed class name to 'active'. themes should use these css classes

th .active
td .active

- tidied up the is_array($header) stuff a bit. It is still present though because we want to skip cell highlighting for tables which don't have headers.

new patch. i had missed one instance of class='cell-highlight'

This patch is a _lot_ better but seems to overwrite existing 'td classes'. Take a look at the watchdog table.

attached patch adds onto existing TD classes instead of overwriting them.

even better this time. tables without sortable headers no longer get the 'active' attribute ...

DRUPAL.CSS
added italics to sorted column. may be further defined in theme css. use the css classes 'th.active' and 'td.active'. also prevented image in active table header from wrapping.

Committed to HEAD.

Kika, Megagrunt and friends: now all preparations have been made to support 'pretty tables', we expect theme patches to hit the patch queue. Marking this active, not fixed.

attached patch:
- fixes comment.module bug highlighted by kika in previous comment
- move a little tablesort logic from theme_table() to tablesort_x functions.
- fix bug when multiple sortable tables appear on same page

this time with a patch actually attached

Committed to HEAD. Thanks Kristjan and Moshe.

Committed to HEAD.

Possible interactions with class="active"?

I am having problems getting a:active{} to work with Drupal in any browser though I can get it to work in other sites. Is it possible that your use of class="active" is interfering with this proper functioning of this CSS code?

Also

When I try to use something like .content .active {color:#ff0000} to control an elements appearance the settings in .content .active a:visited{} override those settings. I know you are supposed to use "a.content" but that syntax does not work when I try to use it.

Is it possible that your use of the class "active" is conflicting with a:active and causing this?

I am going to post this else where since this thread seems to be over...

Keith