Prioritizes people autocomplete suggestions #762
Conversation
|
Great. I like it that you wrote tests for that. I would say it is good enough to merge, but I think the code can be improved from stylistical point of view, so why not do it (I'll write my comments in a code review next) |
There was a problem hiding this comment.
It seems I found a good amount of things to change in the end 😄
I think these are quite interesting things to learn though.
Add few more tests... even add them before you implement the feature.
For example, add a test for duplicate items. Then once you have a failing test, make it pass by implementing this feature.
src/users/usersSelectors.js
Outdated
| users.map((user) => { | ||
| if (user.email === ownEmail) return user; | ||
| else if (user.fullName.toLowerCase().startsWith(filter.toLowerCase())) { | ||
| startWith = [...startWith, user]; |
There was a problem hiding this comment.
This is not performant. On every match it will create a new array.
src/users/usersSelectors.js
Outdated
| export const filterUserList = (users: any[], filter: string = '', ownEmail: ?string): any[] => | ||
| users.filter(user => | ||
| user.email !== ownEmail && | ||
| (filter === '' || | ||
| user.fullName.toLowerCase().includes(filter.toLowerCase()) || | ||
| user.email.toLowerCase().includes(filter.toLowerCase())) | ||
| ); | ||
|
|
||
| export const alphabeticallySort = (users: any[]): UserType[] => |
There was a problem hiding this comment.
A better name would be 'sortAlphabetically' 🥇
There was a problem hiding this comment.
Type for the parameters would be users: UserType[]
src/users/usersSelectors.js
Outdated
| let initials = []; | ||
| let contains = []; | ||
| let matchesEmail = []; | ||
| users.map((user) => { |
There was a problem hiding this comment.
You can use users.filter
src/users/usersSelectors.js
Outdated
| else if (user.fullName.toLowerCase().startsWith(filter.toLowerCase())) { | ||
| startWith = [...startWith, user]; | ||
| return user; | ||
| } else if (user.fullName.replace(/(\s|[a-z])/g, '').toLowerCase().startsWith(filter.toLowerCase())) { |
There was a problem hiding this comment.
Extract each of these to 4 functions that are in the form of
users.filter(user => ....)
then this function will ony be
const usersByStartWith = getUsersByStartWith(users, filter);
const usersByinitials = getUsersByInitials(users, filter);
....
return [...startWith, ...initials, ...contains, ...matchesEmail];I think a deduplication phase will be needed in the end though.
You can extract it to a getUniqueItems() function
There was a problem hiding this comment.
Since there will be duplications between the 4 lists, you can use this:
https://stackoverflow.com/questions/31740155/lodash-remove-duplicates-from-array
- starts with fullName. - matches initial. - contains in name. - matches in email. in a single iteration of users array. fix:zulip#756
|
@borisyankov updated 👍 |
|
Awesome. The reworked version is exactly what I meant 🥇 |
Prioritizes people autocomplete in order of :-
in a single iteration of users array.