-
-
Notifications
You must be signed in to change notification settings - Fork 833
apply roomlist searchFilter to aliases if it begins with a #
#1957
Conversation
Signed-off-by: Michael Telatynski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what's with the h1 on 'Fixes' in the description?
// to prevent loads of false positives with server names, e.g `matrix` | ||
if (filter[0] === '#' && room.getAliases().some((alias) => { | ||
return alias.toLowerCase().startsWith(lcFilter); | ||
})) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fact that the conditional is spread over 3 lines disqualifies the from being a line-if, so I'd prefer if we just put the braces in here I think.
return room.name && room.name.toLowerCase().indexOf(filter.toLowerCase()) >= 0 | ||
if (room.name && room.name.toLowerCase().includes(lcFilter)) return true; | ||
// only apply search filter to aliases if it looks like an alias (starts with `#`) | ||
// to prevent loads of false positives with server names, e.g `matrix` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wouldn't cause false positives from server names though since you use startsWith? It would just never match anything if the filter didn't start with '#' so this is a fast-path optimisation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be .includes
like the above then I switched it when I added the startsWith# as we know it's start-anchored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove the comment if its too redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back to you
so I/others don't miss Dependencies and which issues it Fixes I've had PRs merged before where it said "Depends on js-sdk#XXXXX" and yet the dependency didn't get merged and thus broke /develop |
Signed-off-by: Michael Telatynski <[email protected]>
Fixes element-hq/element-web#5719
applies only if begins with a
#
otherwise yielded too many false positives when searching formatrix
Signed-off-by: Michael Telatynski [email protected]