-
Notifications
You must be signed in to change notification settings - Fork 0
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(frontend): allow filtering variant table on ID #309
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.
Hey Elissa, thanks for tackling this and nice work getting all the plumbing working.
I left one main comment about keeping the linking functionility working, with a suggestion on how to do that.
There's then one minor suggestion of having padding above the list, see comment inline.
I'm sure Sam will appreciate this search, thanks for following up with this when the change to a fixed sized list inadvertently created the need for this new search, and for following through with the highlighting.
frontend/src/components/VariantListPage/VariantListVariants.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/VariantListPage/VariantListVariants.tsx
Outdated
Show resolved
Hide resolved
575a01e
to
e617888
Compare
e617888
to
cfef759
Compare
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.
Hey Elissa, codewise and feature wise, this is looking good!
I'm requesting changes to get a bit more rebasing practice, I think (feel free to disagree) a set of two commits here would be appropriate
- 1 that adds the new dependency
- 1 that adds the feature to search by variant id with the bar
This way, if any bugs arise, we can more confidently say if it was the dependency commit, or the new commit. Here, just adding an NPM dependency is a bit unlikely to cause problems, but if co-dependencys bump versions or something like that happens, it could cause problems.
I think the practice of keeping bumps to dependencies or tools in isolated commits will pay off in the long run, so I'm just requesting one more interactive rebase to turn this into the two commits proposed above (again, feel free to disagree if you think there should be more or less commits, you'll get a feel for this over time).
cfef759
to
d0a6c04
Compare
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.
LGTM!
Fixes #294
Creates a search feature for the variant table that filters by variant ID. Matching text from the search query is then highlighted within the table and the list scrolls to the first matched variant row to maintain context (similar to the gnomAD "display neighboring variants" search feature)