| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | theme system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
The first argument of theme_table ($header) consists of a structured array for each column header.
The 'sort' value supposedly allows to set a default sort order for this column. This would be very convenient - simply set this to 'desc', and from now on, a click on that header will first sort the entries in descending order. This saves an extra page load in fields that are routinely sorted in descending order.
However, instead of actually changing the default sorting direction for that field, it seems to set the current sort order to that field and that direction - as indicated by the little triangle next to the column header.
If this is intentional, the API page should probably be rewritten to make this clear. In that case, this is just a feature request for a default sorting order as described above.
Comments
#1
I know now that this is by design, and I think it's also been documented somewhere. However, what I'm still missing is a way to reverse the default direction on a certain field.
Take a table that ranks popularity of a list of items. When someone clicks the Popularity column, they obviously expect the items to be sorted in descending order, because most would want to look at the popular end of the list first (also, in any given one-sided distribution, the maximum is a lot more narrow than the minimum, where you will likely end up with several thousands of 0s). This currently takes two clicks, as the table will first be sorted in ascending order. It's been a pet peeve for me for the past two years, and the subject of uncountable user complaints as well, on each of my sites that exposes sortable tables.
#2
I have a little solution for this, should work in all drupal versions:
you have to add simply one line to /includes/tablesort.inc in function "tablesort_header"
change this
else {// If the user clicks a different header, we want to sort ascending initially.
$ts['sort'] = 'asc';
$image = '';
}
to this:
else {// If the user clicks a different header, we want to sort ascending initially.
//$ts['sort'] = 'asc';
$ts['sort'] = (strpos($cell['data'],'<!--DESC-->') === FALSE) ? 'asc' : 'desc';
$image = '';
}
now drupal checks if the string "" is included in the data field of the header of table.
so you have simply add a header like this to get it initially sorted descent
// initial order desc$header[] = array('data'=>t('vote count<!--DESC-->'),'field'=>'votecount');
// initial order asc
$header[] = array('data'=>t('something else<!--DESC-->'),'field'=>'something');
#3
I agree that the lack of support for this is unfortunate. Probably too late for feature requests in D7, but before changing version to D8, setting component to "database system" in case anyone monitoring that issue queue feels inspired to roll a patch. Clearly, this issue hasn't had much luck as part of the "theme system" queue.
#4
I've tried to come up with something, since it's really not complicated (I hope). New doc comment:
- "first sort": The default sort order for this column, by which the column will be sorted after the first click.
- "sort": For one column, the order by which the table will be sorted by default (i.e., before the user explicitly sorts the table).
Only thing bothering me is the terminology, since "first sort" doesn't really sound right – but I think, "default sort" would be pretty confusing with the current "sort". Swapping those fields would be clearest, I'd say, but of course this would break about every table header definition out there.
So this patch just adds the requested possibility without breaking existing code.
Changing back to "theme system" as this doesn't concern the database at all (TableSort extender just uses whatever it finds in $_GET).
(Not sure about the tag, btw – is there any other documentation on this than the theme_table() doc comment?)
#5
Forgot part of the documentation comment (as always).
- "first sort": The default sort order for this column, by which the column will be sorted after the first click ("asc" or "desc"). Defaults to "asc".
- "sort": For one column, the order by which the table will be sorted by default (i.e., before the user explicitly sorts the table).
#6
I just realized an error – I forgot to unset the "first sort" key after processing, so the resulting tables contained invalid HTML. Which brings me to the conclusion that there should maybe be some tests for valid HTML.
In any case, here is a revised patch which is now hopefully completely correct.
#7
Nice little improvement. Do we have any tables in core that can benefit from this? Would be good to add at least one so we demonstrate usage.
#8
There are a bunch. I think
grep -r "'sort' => 'desc'" .gives a pretty good overview over some of them.For example, nearly all tables in admin/reports are initially sorted descending by some field (date, timestamp, number of hits).
Of course, there may also be some fields which are not used for the default sort and which should be sorted descending when clicked – but these would be harder to find.
#9
Attached is a new patch, with use cases in the statistics module.
#10
Looks good to me. Thanks for the implementation in stats module
#11
I've wanted this feature for years. Huge UX improvement, especially if we can identify more tables.
#12
+++ includes/theme.inc 17 Jul 2010 11:51:22 -0000@@ -1548,7 +1548,11 @@ function theme_breadcrumb($variables) {
+ * - "first sort": The default sort order for this column, by which the
+ * column will be sorted after the first click ("asc" or "desc"). Defaults
+ * to "asc".
+ * - "sort": For one column, the order by which the table will be sorted by
+ * default (i.e., before the user explicitly sorts the table).
This not really intuitive, IMO. I think we should try to come up with different names and better descriptions.
#13
I agree, these names don't really fit. But I don't think that "sort" can reasonably be changed (neither in name, nor in functionality), since this would mean that all sorted tables in core and contrib would have to be adapted to that. And with this premise, "first sort" was the best I could come up with for the other option, as "default sort" would maybe fit better, but be too confusing regarding the "sort" meaning.
I also can't come up with better descriptions, I find them pretty clear already. Maybe just 'The order by which the column will be sorted after the first click ("asc" or "desc"). Defaults to "asc".' for "first sort"?
Is there a tag for attracting the attention of people who are good at terminology and clarity of documentation?
#14
I'm rewriting the descriptions. Maybe "initial sort" might be a better key, but in the end, it's the descriptions that are going to make the difference. I agree we can't change "sort" at this point.
#15
One option for the "initial sort" is to make it a "descending" => TRUE attribute. Since the default sort order is ascending, you'd then add "descending" => TRUE any time you want to change the default behavior. This would have the benefits of a) not having to document the default behavior, and b) not wasting people's time writing "first sort" => "asc".
Here is some suggested documentation for the comments (retaining "first sort"):
#16
Implemented this with the "descending" => TRUE pattern and found two more columns in the statistics module where it made sense to use.
#17
Doesn't change anything from the last patch except the order of the code comments.
#18
Corrected and simplified code comments.
#19
Looks good to me. Using "descending" is a great idea, and the descriptions are also very clear (at least I think so).
Everything seems to work with this patch. I just altered a few minor things in comments and one code expression.
(Lowercase after semicolons, emphasis on "descending" only applying at first click (in code comment),
empty()instead ofisset() && $.)Also, your patches have two "no status" files at the top,
tablesort.first_sort_2.patchandsites/d7head.localhost.#20
Let's have a committer take a look.
#21
Note: This would also fix #839634: Fix up documentation for table sorting, which should therefore be marked as "fixed" if this patch gets committed.
#22
This patch looks like a good idea, but shouldn't this new behavior (first click is descending) have a test?
#23
On closer inspection, sort link generation wasn't tested at all before. But now that there's a test (which hopefully also passes the test bot), can we please commit this?
#24
The last submitted patch, tablesort_descending.patch, failed testing.
#25
That's what I get for incorporating a dangling whitespace issue …
New patch is created, but I think I'll wait until the end of Core Developer Summit. Right now the test bot has got enough to do as it is.
#26
If you have a new patch, please post it...
#27
OK.
#28
Hmmm.
- array('data' => t('Timestamp'), 'field' => 'a.timestamp', 'sort' => 'desc'),+ array('data' => t('Timestamp'), 'field' => 'a.timestamp', 'descending' => TRUE, 'sort' => 'desc'),
How does it make sense to set the descending flag on a particular cell as well as the sort flag?
Because it looks like the first time tablesort_header() is called, the descending flag goes away?
- unset($cell['field'], $cell['sort']);+ unset($cell['field'], $cell['sort'], $cell['descending']);
Something about this isn't making sense to me in the code. ???
And what's this "first sort" thing?
- array('data' => t('Hits'), 'field' => 'hits', 'sort' => 'desc'),+ array('data' => t('Hits'), 'field' => 'hits', 'first sort' => 'desc', 'sort' => 'desc'),
#29
> Something about this isn't making sense to me in the code. ???
As far as I can tell, tablesort_header is called only once per request, so the unsetting at the function's end has no influence on the effect of "descending" and "sort". And really, not specifying both when
'sort' => 'desc'is set doesn't usually make sense. At the moment it's like this:First request => ORDER BY time DESC
Click on column "foo" => ORDER BY foo ASC
Click on column "time" => ORDER BY time ASC
With the patch, sorting on "time" after first changing the sort to something else would again give a descending sort – what is wanted and expected by the user most of the time, I think.
> And what's this "first sort" thing?
'first sort' => 'desc'was the first proposed way of doing'descending' => TRUE– it being still in there is of course wrong, thanks for pointing it out.Attached is a new patch fixing that last thing.
#30
That makes sense about setting both sort and descending...
But if tablesort_header() is only called once, what is the purpose of unsetting those array elements?
#31
After
tablesort_header()is called, the array is used to build the table cell, with all remaining array elements (other than "data") being used as attributes. So if we wouldn't unset it, we'd have something like<td field="time" sort="desc" descending="1">.#32
Ah, that makes sense... Maybe a comment in the code explaining that would be helpful? :)
#33
OK.
#34
I found one spot where coding standards weren't being followed:
array(
array(
item [not followed by comma]
)
)
So I edited the patch to put that in. With that added comma, I think it's all good, well-documented code and a needed feature. Let's see if we can get this in...
#35
Thanks for catching that!
#36
oh, awesome. Is this the cause for d.o sorting issues backwards by default?
#37
#34: 109493-34.patch queued for re-testing.
#38
Yeah, code looks good to me too.
#39
Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).
#40
I'm not sure, it may be. But since your comment lacks any real argumentation, I'm setting it back for now, waiting for other opinions. I don't think this would hurt that badly for D7.
#41
Rules or no, there have been other "nice to have" patches going into D7. Forcing core maintainers to do "X", whatever "X" is (ie stick to rules) is not fruitful business. Until Dries says that only criticals are going in, leave it D7. We can bump it to D8 in due time.
#42
I'm going to postpone this to Drupal 8. We survived Drupal 7 without this, I think we'll survive another release cycle without this feature. It's a feature request.
#43
Subscribing. Could this be one of those non-API-changing additions that goes into 8, and then goes into 7?
#44
Seems to still apply, maybe we can now finally get this in.
#45
I think this is a great usability enhancement with not much overhead and no api consequences. I would love to see added to Drupal 7. Otherwise I (and others) will always have to patch it in on each core update, which is a pain (especially if those core patches sum up)...
#46
Hm, I'm not sure whether you can really say that it has „no API consequences“ … After all, it does add another key to an array in an API function.
Bug fixes like #838096: Problem with the "active" class of tablesort seem more pressing to me, with much the same argument of having to patch this manually.
Getting any of this in would be brilliant, though. These are the things that really suck in core work.
#47
Ok, it has consequences. But these are intentional and optional. I meant consequences for already existing code.
If you add functionality without changing the way a function is called and operates otherwise, then there is no "api change" in this sense. In other words, this will not possibly affect any code that uses that function in the way it is designated.
Besides that, to be precise, it's not quite true that it "does add another key to an array in an API function". It is considering an additional key of a passed array.
#48
I really hope there is some way this can be added to 7.
Just spent a couple of hours trying to figure out why when my header field was set to sort=>DESC it sorts by ASC when clicked. IMO the current extend('TableSort')->orderByHeader($header_row) documentation as is is very confusing, and leads me to believe I should have the functionality of above patch, but in reality I do not.
As commenters above noted, there are lots of cases where we do not want two clicks required to get a DESC sort order. This is a big enough deal for me to consider abandoning extend('TableSort')->orderByHeader($header_row) in favor of a 3rd party table script. However, I would much rather use built in Drupal functionality.
#49
You'll survive it.
(Sorry, couldn't resist...)
#50
I'm a little surprised that this was marked a feature request and not a bug. The documentation and some parts of the code create the expectation that this feature already exists and will work, so it seems like more of a bug to me.
See the docs here: http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_table/7
I'm not sure what the 'sort' key in the header array would be for, if not for setting a default sort direction.
#51
^ +1
#52
It defines the one column by which the table will be sorted by default. But yes, the documentation is very misleading. See #839634: Fix up documentation for table sorting.