Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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!
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | tablesort_header_1.patch | 1.61 KB | Robin Monks |
| #12 | tablesort_header_0.patch | 1.6 KB | Robin Monks |
| #10 | tablesort_header.patch | 1.49 KB | Robin Monks |
| #6 | tablesort-sorting-theme-improvement_0.patch | 1.73 KB | Stefan Nagtegaal |
| tablesort-sorting-theme-improvement.patch | 1.25 KB | Stefan Nagtegaal | |
Comments
Comment #1
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedSetting status to 'Patch' instead of 'Fixed' so it shows up in the patch queue
Comment #2
Dries CreditAttribution: Dries commentedPlease move the proposed theme functions to
includes/theme.inc(so Doxygen includes them in the list of theme_ functions).Comment #3
Steven CreditAttribution: Steven commentedWe should avoid entities like in the code. This can be achieved through CSS instead in the theme.
Comment #4
Steven CreditAttribution: 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).
Comment #5
Dries CreditAttribution: Dries commentedStefan's patch actually removes the entities, not (or is my jet lag worse than it appears)?
Comment #6
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedUpdated patch which removes the and moves the
theme_tablesort_asc()andtheme_tablesort_desc()to theme.inc.This is much better imo than the current approach..
Comment #7
Steven CreditAttribution: Steven commentedYeah 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(), ...
Comment #8
Dries CreditAttribution: Dries commentedI agree with Steven. We should have a function called theme_tablesort_header($style) where $style can be either 'ascending' or 'descending'.
Comment #9
Dries CreditAttribution: Dries commentedComment #10
Robin Monks CreditAttribution: Robin Monks commentedHere 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
Comment #11
Bèr Kessels CreditAttribution: Bèr Kessels commentedI 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.
Comment #12
Robin Monks CreditAttribution: Robin Monks commentedHere is a new patch using if instead of the switch.
Robin
Comment #13
Robin Monks CreditAttribution: Robin Monks commentedThox suggested using indicator instead of header.
Robin
Comment #14
mfb+1 looks good.
Comment #15
Jose Reyero CreditAttribution: Jose Reyero commented+1
Tested the patch -applies with some line shift-, and it works
Comment #16
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #17
(not verified) CreditAttribution: commentedComment #18
(not verified) CreditAttribution: commentedComment #19
(not verified) CreditAttribution: commentedComment #20
(not verified) CreditAttribution: commentedComment #21
(not verified) CreditAttribution: commentedComment #22
(not verified) CreditAttribution: commentedComment #23
(not verified) CreditAttribution: commented