Change tab completion in the UI to prefer common prefix #6759
Conversation
|
@opihana, quick tag as this is interesting for us and our upcoming work |
There was a problem hiding this comment.
Hey Matthew,
I only came to nudge @opihana about this as it interesting UX for us to lean on for some upcoming work, then I thought I'd take a quick look and ended up saying quite a bit 😬 .
I've got a feeling I might have missed the point with this, or maybe I'm just looking at it differently, so take a lot of this with a pinch of salt - see what you/others think.
Where's the :bushes: emoji when you need it!
John
ui/app/utils/common-prefix.js
Outdated
| import { get } from '@ember/object'; | ||
|
|
||
| export default function(arr, attribute = 'id') { | ||
| let content = arr || []; |
There was a problem hiding this comment.
Could you replace this line 4 with a default argument? Nit picky, but you do it with attribute also, up to you.
function(arr = [], attribute = 'id')There was a problem hiding this comment.
In fact looking over the rest I don't think the rest of the function works if arr/content === []? Maybe I read it wrong?
There was a problem hiding this comment.
Ah yep, need an early return - in the app we're checking to make sure there's a match first, but with that adjustment wouldn't need to. Will get that fixed up!
ui/app/utils/common-prefix.js
Outdated
| // this assumes an already sorted array | ||
| // if the array is sorted, we want to compare the first and last | ||
| // item in the array - if they share a prefix, all of the items do | ||
| let firstString = get(content[0], attribute); |
There was a problem hiding this comment.
Is this ok if content[0] === undefined? Not massively sure. (P.S. Also see comment above)
ui/app/mixins/list-controller.js
Outdated
| return re.test(key.get('id')); | ||
| }); | ||
| let matchSet = content.filter(key => re.test(key.id)); | ||
| let match = matchSet.firstObject; |
There was a problem hiding this comment.
Just wondering if you can do matchSet[0] with the new ember proxy things? Does that proxy through properly or do you have to use matchSet.firstObject?
There was a problem hiding this comment.
Also, unless I've misunderstood, you could probably take the opportunity while you are here to make this consistent:
let filter = this.filter;
let content = this.model;
let filterMatchesKey = this.filterMatchesKey;
let re = new RegExp('^' + escapeStringRegexp(filter));Again up to you, just something I noticed, pretty sure you can switch out those gets now but maybe you have a reason to keep them?
There was a problem hiding this comment.
/me nods - I often forget to update all of the surrounds when I'm in an old file
There was a problem hiding this comment.
Cleaned up a lot of this in e0b4c7e - thanks!
| // item in the array - if they share a prefix, all of the items do | ||
| let firstString = get(content[0], attribute); | ||
| let lastString = get(content[arr.length - 1], attribute); | ||
|
|
There was a problem hiding this comment.
I'm confused as to why you only check the last string in the array here, maybe I don't understand the use case? Ah ok hold on I just re-read that comment up above here (lines 5-8). Wonder if it would be nicer to just pass the two strings instead of an array?
function commonPrefix(a = '', b = '') {
...
}
commonPrefix(arr[0].id, arr[arr.length - 1].id)
commonPrefix(arr[0].name, arr[arr.length - 1].name)
// maybe I might want to recur this at some point
function(item) {
commonPrefix(prev.id, item.id)
}You also remove your get dependency by doing that and reduces your function to just do one thing. Again, up to you
There was a problem hiding this comment.
For now I think having the whole array contains the overall logic a bit better - if we later decide to change how we calculate the common prefix, we'll have all of the data here - I don't think it saves us anything because we'd need to pull out those strings somewhere. Good point about the get usage - looking at it, I think we can nix it anyway since we can use ES5 getters - I'll look at that!
| // the longest the shared prefix could be is the length of the match | ||
| let targetLength = firstString.length; | ||
| let prefixLength = 0; | ||
| // walk the two strings, and if they match at the current length, |
There was a problem hiding this comment.
great use of comments in this function, btw. thank you!
This changes the autocomplete behavior on routes that use the
NavigateInputwith the list-controller mixin so that tabbing on the keyboard will first check to see if there is a common prefix. Previous to this, tabbing would complete the first match in the list - this is now only done when there isn't a common prefix for all of the currently listed keys.In the example gif we see that hitting tab on the full list auto-completes "one" - this behavior is the same as before. What's new is if you filter the list (in the gif typing
sec) and then tab, the completion fills tosecret-and then will completesecret-oneon a second tab. This behavior is closer to how autocomplete works on the Vault CLI.