[drupal-devel] [bug] Theming tablesort icons is inconsistent

[mfb]

Issue status update for http://drupal.org/node/11927

 Project:      Drupal
 Version:      cvs
 Component:    theme system
 Category:     bug reports
 Priority:     normal
 Assigned to:  Robin Monks
 Reported by:  stefan nagtegaal
 Updated by:   mfb
 Status:       patch

+1 looks good.




mfb



Previous comments:
------------------------------------------------------------------------

October 22, 2004 - 09:56 : stefan nagtegaal

Attachment: http://drupal.org/files/issues/tablesort-sorting-theme-improvement.patch (1.25 KB)

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!




------------------------------------------------------------------------

October 22, 2004 - 09:57 : stefan nagtegaal

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




------------------------------------------------------------------------

October 23, 2004 - 09:27 : Dries

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




------------------------------------------------------------------------

October 23, 2004 - 13:39 : Steven

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




------------------------------------------------------------------------

October 23, 2004 - 13:41 : Steven

(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).




------------------------------------------------------------------------

October 23, 2004 - 17:08 : Dries

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




------------------------------------------------------------------------

October 24, 2004 - 03:56 : stefan nagtegaal

Attachment: http://drupal.org/files/issues/tablesort-sorting-theme-improvement_0.patch (1.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..




------------------------------------------------------------------------

October 24, 2004 - 08:10 : Steven

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(), ...




------------------------------------------------------------------------

November 15, 2004 - 05:25 : Dries

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




------------------------------------------------------------------------

June 20, 2005 - 05:43 : Robin Monks

Attachment: http://drupal.org/files/issues/tablesort_header.patch (1.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




------------------------------------------------------------------------

June 20, 2005 - 05:47 : Bèr Kessels

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.




------------------------------------------------------------------------

June 20, 2005 - 06:32 : Robin Monks

Attachment: http://drupal.org/files/issues/tablesort_header_0.patch (1.6 KB)

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


Robin




------------------------------------------------------------------------

June 20, 2005 - 06:43 : Robin Monks

Attachment: http://drupal.org/files/issues/tablesort_header_1.patch (1.61 KB)

Thox suggested using indicator instead of header.


Robin