Skip to content

Add options to sort the listbox items by given sort funcs on update of listbox - #81

Open
ute-arbeit wants to merge 3 commits into
maykinmedia:masterfrom
ute-arbeit:master
Open

Add options to sort the listbox items by given sort funcs on update of listbox#81
ute-arbeit wants to merge 3 commits into
maykinmedia:masterfrom
ute-arbeit:master

Conversation

@ute-arbeit

Copy link
Copy Markdown

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

@CharString CharString left a comment

Copy link
Copy Markdown

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.

Comment thread src/dual-listbox.js
if (a.textContent == b.textContent) return 0;
return 1;
});
}

Copy link
Copy Markdown

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
Copy Markdown
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