-
-
Notifications
You must be signed in to change notification settings - Fork 658
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
feat: more powerful project search #4542
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
a.localeCompare(b) | ||
), | ||
options: [...new Set(filterOptions)] | ||
.filter((it: unknown) => it) |
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.
filter out empty suggestions
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.
Is this the same as .filter(Boolean)
? Maybe that would be cleaner.
row: IFeatureToggleListItem, | ||
values: string[] | ||
) { | ||
return includesFilter( |
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.
Example:
tag1:val1
tag2:val2
tag3:val3
We search if any of the lines includes e.g. tag2:val2
}, | ||
filterParsing(value: string) { | ||
// only first tag from the list is added to search suggestions | ||
return value.split('\n')[0]; |
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.
when returning filtering hints we return only first tag if we have more than one
@@ -12,32 +12,37 @@ type IUseSearchOutput<T extends any> = { | |||
getSearchContext: () => IGetSearchContextOutput<T>; | |||
}; | |||
|
|||
const normalizeSearchValue = (value: string) => |
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.
we normalize search and filter data before any processing. This normalization allows to add spaces around commas
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.
Bit unsure about the behavior from a user-perspective but maybe it's just me.
I guess the natural progression of this for spaced-values would be to wrap them in double quotes. Something like tag:tag1, tag2, "My Value With Spaces", another
🤔
The code looks solid on a first glance 👍
About the changes
Project search improvements:
Make search more forgiving:
col1:val1 , val2
(spaces around comma)col1:value1, value2 col2: value3, value4
Adding extensive test coverage for the search hook. It simulates what people type into search so it should be easy to reason about and replicate different corner cases.
Important files
Discussion points
I'm still not sure if it's a good idea that we require no space between
col1:val1
. I'd like to supportcol1: val1
with a space.Also I'm not sure how we should handle columns values with spaces (e.g. tag value can contain spaces)