Add fuzzy matching support in room searches #6182
Conversation
|
|
||
| constructor() { | ||
| constructor(options: IOpts = {}) { |
There was a problem hiding this comment.
not sure how we feel about this more generally, but making this private fuzzy = false is slightly cleaner and works well with IDEs. The options approach does allow for expandability, however this is internal API and can be changed with relative ease.
There was a problem hiding this comment.
I'm not sure if I understand the change that you're suggesting.
As far as I can tell I have created private fuzzy = false and this options parameter is only here to let a developer choose the type of matching the NameFilterCondition should apply
Could you show me an example of what you had in mind instead?
There was a problem hiding this comment.
In this case, merging the field declaration with the constructor declaration: constructor(private fuzzy = false) {
It's a little bit cleaner for the API contract this part of the code is after.
| import { fuzzyMatch } from "../../src/utils/strings"; | ||
|
|
||
| describe("strings", () => { | ||
| describe("fuzzyMatch()", () => { |
There was a problem hiding this comment.
a test that deals with numbers and symbols interspersed with characters would be nice as well, to ensure the regex cleaning works (of which there doesn't appear to be any?) and that it handles non-ascii well (including emoji).
There was a problem hiding this comment.
Makes sense, I have added more test cases to deal with more exotic character set.
I'm not sure if I know what regex cleaning is and what you expect it to do in this scenario?
There was a problem hiding this comment.
"regex cleaning" is my horrible way of referring to the action the regex performs. There should be tests to make sure that the regex does the right thing, where the "right thing" is cleaning/sanitizing the input from what I can see.
|
@turt2live bump, can you help clarify some of your comments above please |
|
Sorry, I had this one slip through the notifications. A mention or review request will always cut through the sea of notifications I deal with. |
|
@gsouquet can you please update this ticket to describe the changes you've made so that we can review them from a product and design perspective? |
|
There are no changes done to the UI at the moment. What was originally discussed with Amsha is that fuzzy matching is going to yield more results. So we need to agree on a visual representation of what has "matched", and how to sort those results. One caveat with the sorting is that the room list tags (categories) already provide an option to update the sorting of tiles (either alphabetically or by last activity)
Filtering with fuzzy matching would potentially override that sorting to bump the more relevant tiles to the top, which could be quite jarring. But that is definitely something to explore and see what would make the most sense with our current design |
|
Yeah, I think it's worth considering to show search results by relevance alone rather than based on activity or A-Z, given that that's how most search works on different platforms. |
|
This pull request is now stale and should handle the new search experience. We're also not doing this as part of the work to get the new search experience out of beta |
Fixes element-hq/element-web#14410
This adds support for fuzzy matching in room searches
This is a rather simplistic approach to it. There might be some UX changes to this, I have open a discussion on the #element-dev:matrix.org channel