Attached is a patch for tablesort.inc much cleaner, friendlier and more consistent way to theme the tablesort icons.
Marking as a bug becuase it is inconsistent with the rest of drupal..

Please review and apply!

CommentFileSizeAuthor
#13 tablesort_header_1.patch1.61 KBrobin monks
#12 tablesort_header_0.patch1.6 KBrobin monks
#10 tablesort_header.patch1.49 KBrobin monks
#6 tablesort-sorting-theme-improvement_0.patch1.73 KBStefan Nagtegaal
tablesort-sorting-theme-improvement.patch1.25 KBStefan Nagtegaal

Comments

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented

Setting status to 'Patch' instead of 'Fixed' so it shows up in the patch queue

dries’s picture

dries commented

Please move the proposed theme functions to includes/theme.inc (so Doxygen includes them in the list of theme_ functions).

Steven’s picture

Steven commented

We should avoid entities like   in the code. This can be achieved through CSS instead in the theme.

Steven’s picture

Steven commented

(ack submitted too early)

Also, I wonder about the granularity of these theme functions. I don't see the point in putting ascending/descending in a separate function: they are different styles for the same element (the tablesort indicator).

dries’s picture

dries commented

Stefan's patch actually removes the entities, not (or is my jet lag worse than it appears)?

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented
StatusFileSize
new tablesort-sorting-theme-improvement_0.patch1.73 KB

Updated patch which removes the   and moves the theme_tablesort_asc() and theme_tablesort_desc() to theme.inc.

This is much better imo than the current approach..

Steven’s picture

Steven commented

Yeah the entity was actually a +1... I should stop doing so much things at the same time. My other concern is still valid though: theme functions should theme page /elements/, not versions of that element.
Otherwise we would have theme_node(), theme_node_sticky(), theme_node_teaser(), ...

dries’s picture

dries commented

I agree with Steven. We should have a function called theme_tablesort_header($style) where $style can be either 'ascending' or 'descending'.

dries’s picture

dries commented
robin monks’s picture

robin monks commented
Assigned: Stefan Nagtegaal » robin monks
StatusFileSize
new tablesort_header.patch1.49 KB

Here is a new patch with theme_tablesort_header($style) that takes either asc or desc as arguments.

Tested to work on Drupal CVS HEAD.

Robin

Bèr Kessels’s picture

Bèr Kessels commented

I am not sure about teh switch; AFAIK there will be only asc and desc. So an if would be enough. Saves some lines of code, like "break"s; etc.

robin monks’s picture

robin monks commented
StatusFileSize
new tablesort_header_0.patch1.6 KB

Here is a new patch using if instead of the switch.

Robin

robin monks’s picture

robin monks commented
StatusFileSize
new tablesort_header_1.patch1.61 KB

Thox suggested using indicator instead of header.

Robin

mfb’s picture

mfb
they or he
commented

+1 looks good.

jose reyero’s picture

jose reyero commented

+1
Tested the patch -applies with some line shift-, and it works

dries’s picture

dries commented

Committed to HEAD. Thanks.

Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)