Add a component for filtering repositories based on their results#2343
Add a component for filtering repositories based on their results#2343robertbrignull merged 13 commits intomainfrom
Conversation
shati-patel
left a comment
There was a problem hiding this comment.
Code looks sensible, and works nicely locally! ☑️
I have a couple of non-blocking questions/suggestions, but LGTM overall 😎
| }, | ||
| ); | ||
|
|
||
| it("returns true if filterKey is all and resultCount is positive", () => { |
There was a problem hiding this comment.
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.
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.
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.
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
| return undefined; | ||
| } | ||
|
|
||
| // If repository IDs are given, then ignore the search value and filter key |
There was a problem hiding this comment.
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.
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. |
elenatanasoiu
left a comment
There was a problem hiding this comment.
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-reviewlabel there.