-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add options to sort the listbox items by given sort funcs on update of listbox #81
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A consideration is that sorting with a compare function does many comparisons; sometimes even the same elements more than once. And that this API is not very extensible; true or false to signal a hardcoded operation. It does have the benefit of being simple to use, but what if my texts are ['One', 'Two', 'Three'], then numeric sorting on their values may make more sense.
Asking a key function and doing a map, sort, map would solve both of these "issues":
let keyfunc = (e) => e.textContent; // this should be exposed in the api or injected
let sorton = elements.map((e, i) => {index: i, value: keyfunc(e)});
// Then do a comparison that makes sense for the type
sorton.sort((a, b) => a.value.localeCompare(b.value)); // for strings
sorting.sort((a, b) => a > b ? 1 : a < b ? -1 : 0; // otherwise?
let sortedElements = sorton.map((e) => elements[e.index];
But this may be overly complex[1] and premature optimization. If not, and we are going to complexity town, then this may be the time to optimize the visible DOM manipulation by adding a single DocumentFragment in one go, instead of calling list.appendChild
in a loop.
[1] The map, sort, map is actually less complex in the sense of algorithmic time complexity; O(n) vs O(n log n). ☺ I'm using complex in the sense of it being complicated.
#50 suggests to me that some ordering function may be extremely expensive (server call-response) and this optimization is actually needed.
So the remark I made below is the simple fix (and some tests are needed!). But the simple fix may not be sufficient to handle all cases we already know of. So before adding tests, better to await more discussion.
if (a.textContent == b.textContent) return 0; | ||
return 1; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This compare function doesn't necessarily do what it expected:
> a = ['Éclair', 'Strasse', 'Dachshund','Straße', 'sträflich', 'strafe', 'Strafe']
> a.sort(function(a, b) {
... if (a < b) return -1;
... if (a == b) return 0;
... return 1;
... });
[
'Dachshund',
'Strafe',
'Strasse',
'Straße',
'strafe',
'sträflich',
'Éclair'
]
Better would be
> a.sort((a, b) => a.localeCompare(b))
[
'Dachshund',
'Éclair',
'strafe',
'Strafe',
'sträflich',
'Strasse',
'Straße'
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right. What I did was a quick hack to fit my needs for a project.
What do you think about having the sort funcs as options? I committed such a change as well as another one implementing your idea with the DocumentFragment.
Add 'sortAvailableListItems' and 'sortSelectedListItems' options to sort the items alphabetically before adding them again in _updateListbox.