Fix: default table sort was always ascending #444
Conversation
An ascending sort order is always triggered when then target index is different from the tablesort index. This was always true on a page (re)load, because index is initialized with null. By setting the instance' index to the default-sort column index we assure that the direction parameter to sort() is used. See https://github.com/kylefox/jquery-tablesort/blob/master/jquery.tablesort.js#L42
|
Tablesort will not work properly with DataTables that is initialized in _macros.html. If you want to save the sort order on page reload, use stateSave option in DataTables in _macros.html. |
|
I'm not trying to save anything. Maybe it's just me, but right now the default sorting on the index page is broken. I assume the only logical ordering is descending by report timestamp, right? And this PR should address this. |
|
It should or it does? @octomike |
|
Not for me... This is current master:
On month changes this would look even worse, because the ordering is not just in the wrong direction, it's also broken. This is the feature branch:
|
puppetboard/static/js/tables.js
Outdated
| $('thead th.date').data('sortBy', function(th, td, tablesort) { | ||
| var tdTime = new Date(td.text().replace("-", "")); | ||
| if(isNaN(tdTime)) return 0; | ||
| else return tdTime; | ||
| }); | ||
|
|
||
| if ($('th.default-sort').data()) { | ||
| $.tablesort.DEBUG = false; |
There was a problem hiding this comment.
Is this set to true by default?
puppetboard/static/js/tables.js
Outdated
| if ($('th.default-sort').data()) { | ||
| $.tablesort.DEBUG = false; | ||
| var tablesort = $('table.sortable').tablesort().data('tablesort'); | ||
| // explicitly set the current index so we don't get default sort (asc) |
There was a problem hiding this comment.
This comment is relevant to the commit but I don't think it's necessary in the source.
You should change as little as possible. Don't move line 9 to line 15 if you don't have to.
The commit should simply remove line 10 and replace it with:
var tablesort = $('table.sortable').tablesort().data('tablesort');
tablesort.index = $('th.default-sort').index();
tablesort.sort($("th.default-sort"), 'desc');
Take a look at the "Files changed" tab.
There was a problem hiding this comment.
The reordering of the two calls is necessary unfortunately.
As for the annotations, I just removed them and force pushed.
|
As for the fix in end behavior. I can confirm that this change fixes the default sorting when you first launch a page. Nice work 👍 |
octomike
force-pushed
the
fix_default_table_sort
branch
from
45a128b
to
c90d9b0
Compare
|
Meh, I force pushed and I guess that deleted the review comments. Sorry about that. The reordering of the function calls are necessary for the fix, however. |
|
Are there still issues with the proposed fix? |
|
Can this be merged? |
| var tablesort = $('table.sortable').tablesort().data('tablesort'); | ||
| tablesort.index = $('th.default-sort').index(); | ||
| tablesort.sort($("th.default-sort"), 'desc'); | ||
| } |
There was a problem hiding this comment.
IMO $('th.default-sort') should be assigned to a variable to avoid looking up the same selector three times; I imagine that lookup could be fairly slow.
I'd much rather fix issues beforehand -- my experience is that if we don't fix it now, it'll never be fixed, and for minor things like this it's doubly true. @larsnaesbye care to give it a spin and see if everything is alright? I've rebased @octomike's work onto master, done some linting and fixed the requested change. Find it here: https://github.com/runejuhl/puppetboard/tree/pr/444 |
|
I say go for a merge. |
An ascending sort order is always triggered when then target index is different
from the tablesort index. This was always true on a page (re)load, because
index is initialized with null. By setting the instance' index to the
default-sort column index we assure that the direction parameter to sort() is
used.
See https://github.com/kylefox/jquery-tablesort/blob/master/jquery.tablesort.js#L42