Fix 364 #433
Fix 364 #433
Conversation
|
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv |
There was a problem hiding this comment.
Thanks @kwcantrell! Some suggestions -- main thing is that it looks like the functionality added is still commented out, and should be re-enabled. Also, I think the null-filtering should probably be re-enabled, as well.
|
Thanks so much @kwcantrell! Going through the updates now :) @antgonza Quick question -- is it possible to send a "kick"/trigger message to McHelper? Going by the provenance / functionality it looks like the QZVs for this PR haven't been updated in response to the recent commits to this branch, which is making manual testing a bit more cumbersome here. (We can discuss tomorrow morning if you'll be at the meeting, no worries)
|
| // not all node ids were listed in the autofill box | ||
| // create an elipse autofill to let users know there are more | ||
| // possible options | ||
| if (suggestionsAdded < ids.length) { |
There was a problem hiding this comment.
It looks like the ... shows up even if there is only one value shown in the menu -- I think this is because there is only one possible "option" here, i.e. the 0.000 name, so suggestionsAdded for the query 0.000 is immediately less than ids.length. I thought at first that this might be a result of the unique-filtering code you added, but from going through the code I don't think that's the reason.
I think (?) there should instead be some logic that distinguishes between when we already have 10 suggestions added and we had to cut things off (in which case we should show the ...), and the other cases (e.g. the query is so specific that 10 or fewer items are shown, in which case we should not show the ...)
|
@fedarko, my guess is that |
added @fedarko suggestions Co-authored-by: Marcus Fedarko <[email protected]>
empress/support_files/js/empress.js
Outdated
| @@ -443,6 +443,7 @@ define([ | |||
| // specified name in the Newick file) in the auto-complete. | |||
| nodeNames = nodeNames.filter((n) => n !== null); | |||
| nodeNames.sort(); | |||
There was a problem hiding this comment.
https://stackoverflow.com/a/9645447/10730311 -- we should sort case insensitively
There was a problem hiding this comment.
Some tiny comments, but this is pretty much fine as is. Thanks so much @kwcantrell!
empress/support_files/js/empress.js
Outdated
| nodeNames.sort(function (a, b) { | ||
| return a.localeCompare(b, "en", { sensitivity: "base" }); | ||
| }); | ||
| console.log(nodeNames); |
There was a problem hiding this comment.
Could you remove the console.log here?
Also -- do you think it might make sense to move some of this code to a new utility function that just takes a list of node names and filters nulls, sorts it case-insensitively, and filters it to unique stuff? This way, it'd be a lot easier to test this functionality directly. (I mean it works now, so it's not a big issue or anything, but it would probs make the code a bit cleaner.) Not something we have to do in this PR, necessarily.
| nodeNames.sort(); | ||
| nodeNames.sort(function (a, b) { | ||
| return a.localeCompare(b, "en", { sensitivity: "base" }); | ||
| }); |
There was a problem hiding this comment.
Cool! No idea this existed, this seems like a good way of handling this.
Resolves #364. This implements a binary search and also limits the number of suggestions to 10 ids
Here is a link to the improved version and here is a link to the original version.
You can test them by searching for 'taag' in the node search box.