Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Implement filtering in search page #37909
perf: Implement filtering in search page #37909
Changes from all commits
4c017a6
3cda73b
c2e3157
bd323a0
4a4ba24
74a2ecd
d2b234c
39b5036
b04b4ae
67e5dc8
7c2c077
d5a8f97
450c5f0
b71dbea
104b658
4dfeee1
8ea3f82
d9d0e84
32a7100
4702bdc
790a191
8099082
40a9715
3223b35
15ac693
5fbc428
f370b02
18e28c2
8d070e4
6636270
48fddbd
99a0cd7
671c0d8
eb4c265
ae77968
dea2fb3
528ff86
b0bd01d
eab2574
f81bbea
765ae52
f962092
e3b3b97
5ea71db
51771d9
a82c147
fae57e2
78cb949
480f3be
850dd5b
0682936
efedd80
f4fdacd
f94881e
7620b35
5b42f38
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
probably you can initialize it outside of the function
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.
as it is not used anywhere else, I'd leave it here so it's clear where and why this regex belongs to
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.
rather than using a regex to replace dots in emails, can we just have
filterArrayByMatch
ignore dots, semicolons, dashes, etc... by default?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.
hmm, I think it might negatively affect the performance, because we would have to call it for each value under a specified keys. With current approach, we can limit those checks only to the keys we need.
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'm a bit confused why we're doing a
reduce
here, is it intentional that we callfilterArrayByMatch
separately for every search term? doesn't think mean we're filtering over all the results iteratively, once for each search term? Wouldn't it be more efficient to filter all the results once, accounting for all the search terms in a single pass?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 it might be hard to achieve, even current implementation loops through all the search terms.
By default
filterArrayByMatch
returns items where the properties matches the entire search term. For example, when searching forFoo Bar
, it will return only those results whereFoo Bar
exists in a given keys. Withreduce
, we can match bothFoo
andBar
separ ately and return rows where two words are found not only in the same column, but also in different columns.A good example from the app is searching for
Tomasz Callstack
. If we would look for the whole phrase, we would get 0 results. But withreduce
, we get all the options that matches both terms. That's becauseTomasz
was found e.g. in adisplayName
andlogin
properties, andCallstack
was found as a part of an email address in alogin
property.Additionally, note that with every next term, we are filtering the results narrowed to those matching the previous search terms.
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.
would be nice to leave a comment that this is a slim version of what's available in
match-sorter