-
Notifications
You must be signed in to change notification settings - Fork 191
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
Add a component for filtering repositories based on their results #2343
Conversation
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.
Code looks sensible, and works nicely locally! ☑️
I have a couple of non-blocking questions/suggestions, but LGTM overall 😎
@@ -42,6 +43,60 @@ describe(matchesFilter.name, () => { | |||
).toBe(matches); | |||
}, | |||
); | |||
|
|||
it("returns true if filterKey is all and resultCount is positive", () => { |
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.
Minor: this test block is quite large, I wonder if it makes sense to chunk it up into describe
blocks somehow? E.g. the first tests (not the ones you added here) are all about searching, while these new ones describe "when filterKey is all/withResults"
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've split it up into two describe
blocks. Let me know if that wasn't quite what you meant.
}); | ||
}); | ||
|
||
describe("when sort key and search and filter withResults are given", () => { |
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.
Minor: I found this a bit difficult to parse 😅
Maybe something like
describe("when sort key and search and filter withResults are given", () => { | |
describe("when sort key, search, and filter withResults are given", () => { |
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.
Good point. Those definitely could be clearer 😅
I've updated the tests that have at least three items in the list to use commas.
extensions/ql-vscode/test/unit-tests/variant-analysis-filter-sort.test.ts
Outdated
Show resolved
Hide resolved
repositories: T[] | undefined, | ||
filterSortState: RepositoriesFilterSortStateWithIds | undefined, | ||
): T[] | undefined { | ||
if (!repositories) { | ||
return undefined; | ||
} | ||
|
||
// If repository IDs are given, then ignore the search value and filter key |
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.
Minor: Just a question around IDs, since I don't really understand where they come in to play with filtering 😅 When would repository IDs "be given"? As in, where do they come from?
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.
Good question! I believe the answer is whenever you're operating on a list of repos selected by the user, such as when exporting results or copying a repo list to the clipboard. A user can use the tickboxes to select repos, and we pass that list into here using the repository IDs.
Also added a changelog entry because I forgot one until now. |
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.
Looks good 👍
Thanks for splitting up the commits. It was easy to follow along.
This PR adds a new dropdown in the variant analysis results view, to control filtering of repositories. This allows filtering to only those repositories that have results, and is treated separate from the existing search bar which allows filtering based on the repository name.
I've tried where at all possible to use the term "search" when referring to the existing feature of filtering based on the repository name, and used the term "filter" to refer to the new feature of filtering based on results.
The default value is "All" and therefore the default behaviour is identical to the existing behaviour. In later PRs we'll look at changing the default behaviour to "With results" and making it configurable.
I haven't added any kind of feature flag for this because I think we can implement it in shippable chunks, where each individual PR isn't too large and could be shipped on its own. If you think this PR is too large or you'd prefer to have a feature flag regardless, just let me know and we can look at that. This PR contains all the parts, including adding the UI and the backend implementation, and unit tests and a storybook story. I've tried to split up the commits as much as I could so they should all be reviewable in separately.
Checklist
ready-for-doc-review
label there.