Skip to content
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 column-based filters for mutation tables #3831

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

Rajat-Sirohi
Copy link
Contributor

@Rajat-Sirohi Rajat-Sirohi commented Jul 2, 2021

Fix cBioPortal/cbioportal#8713 and 2.2 Filtering per column in cBioPortal/cbioportal#8711

Changes:

  • Added two types of data filters, NumericalFilter and CategoricalFilter, which are attached to columns in the results view mutation table
  • Numerical filters consist of a range slider with two handles and input boxes
  • Categorical filters consist of search and multi-select functionality
  • Multiple column filters can be active simultaneously, and they work in conjunction with the search filter in the top-right (i.e. only those rows which match ALL active filters will be shown)
  • Hover cursor over column in order to show filter icon (if it's applicable for that column)

numerical_filter
categorical_filter

@Rajat-Sirohi Rajat-Sirohi force-pushed the add-col-by-col-filtering branch 2 times, most recently from 21d6153 to ccace2e Compare July 2, 2021 19:11
@Rajat-Sirohi Rajat-Sirohi changed the title Add numerical filters Add column-based filters for mutation tables Jul 6, 2021
@Rajat-Sirohi Rajat-Sirohi force-pushed the add-col-by-col-filtering branch 3 times, most recently from 5dca22a to 60e8b21 Compare July 12, 2021 20:51
@Rajat-Sirohi Rajat-Sirohi marked this pull request as ready for review July 12, 2021 20:52
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for changing the design to use DataFilters! Added some suggestions to further improve the clarity of the filters.

src/pages/resultsView/ResultsViewPageStore.ts Outdated Show resolved Hide resolved
src/shared/lib/MutationUtils.tsx Outdated Show resolved Hide resolved
src/shared/lib/MutationUtils.tsx Show resolved Hide resolved
src/shared/lib/MutationUtils.tsx Outdated Show resolved Hide resolved
env/custom.sh Outdated Show resolved Hide resolved
@Rajat-Sirohi Rajat-Sirohi force-pushed the add-col-by-col-filtering branch 6 times, most recently from c32c454 to b091486 Compare July 15, 2021 16:07
Copy link
Member

@Luke-Sikina Luke-Sikina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small code changes, + 2 more commends:

  • The regex filter shouldn't be case sensitive.
  • The hide NA values check box isn't aligned with the text, that should be fixed.
    Screenshot from 2021-07-15 15-28-07

@Rajat-Sirohi Rajat-Sirohi force-pushed the add-col-by-col-filtering branch 3 times, most recently from d3c3f2e to 3f30f3c Compare July 16, 2021 00:36
@jjgao
Copy link
Member

jjgao commented Jul 16, 2021

Great job! Thanks, @Rajat-Sirohi

@Rajat-Sirohi @Luke-Sikina can we add one additional feature either part of this or or a separate one? It would be really useful to add a link to the study view for the filtered samples.

image

@jjgao
Copy link
Member

jjgao commented Jul 16, 2021

Issue with Exon column in this query

image

@Rajat-Sirohi
Copy link
Contributor Author

Rajat-Sirohi commented Jul 16, 2021

Thanks for identifying that bug, @jjgao ! It was a problem with computing the min/max for the column, where the Exon column was returning a fraction, not a valid number. I've now fixed it and will update the code in the next push.

@Rajat-Sirohi Rajat-Sirohi force-pushed the add-col-by-col-filtering branch 4 times, most recently from 71fc949 to 32aa703 Compare July 22, 2021 17:46
@inodb
Copy link
Member

inodb commented Jul 22, 2021

@Rajat-Sirohi i noticed one tiny bug where the filter icon won't show up. If I go here, mouse over the protein change column header, click on the fliter icon and search for 1000, then click anywhere on the page. The blue filter icon doesn't show up:

Screen Shot 2021-07-22 at 4 45 08 PM

Once I mouse over again, it does show up. Doesn't really have to be a blocker for merging tho, but curious if there's an easy solution

One separate other tiny thing is that the filtering icon shows up even if you are quite far away from the column. Could be considered a feature but to me it seems more clear to only show the icon when you mouseover the column header:

Screen Shot 2021-07-22 at 4 50 35 PM

@Rajat-Sirohi
Copy link
Contributor Author

@inodb The first issue happens because the input box is over a different column, so the filter icon isn't shown until that column is moused over again. I'll definitely see what I can do, but I don't think there's an easy fix.

To your second point, I just check the horizontal coordinates of the cursor and see if it's inside a column, and if so then I show the filter icon. I don't check vertical coordinates because I was told to show it if the mouse is inside any part of the column, not just the header. Unfortunately I also end up showing it if the mouse is above the column due to an issue where the vertical position of the column header sometimes changes. I don't think there's an easy fix for that either.

@Rajat-Sirohi
Copy link
Contributor Author

@inodb I've now fixed that first issue so that the filter icon is visible so long as the cursor is either over the column OR the filter menu is open. Thanks for pointing it out.

I think I will leave the other part as is, where the icon is shown regardless of the vertical position of the cursor. This might seem unnecessary, especially when the cursor is above the column header, but I think it's better to ensure that the user knows that filtering is an option rather than risk them not even noticing the icon if were to make its visibility so restrictive, e.g., if it were only shown when hovering over the column header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants