Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ute-arbeit
Copy link

Add 'sortAvailableListItems' and 'sortSelectedListItems' options to sort the items alphabetically before adding them again in _updateListbox.

Copy link

@CharString CharString left a 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;
});
}

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'
]

Copy link
Author

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.

@ute-arbeit ute-arbeit changed the title Add options to sort the listbox items by their text on update of listbox Add options to sort the listbox items by given sort funcs on update of listbox Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants