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 splice and driver annotation mutation type selector #3704

Merged
merged 4 commits into from Apr 29, 2021

Conversation

leexgh
Copy link
Member

@leexgh leexgh commented Apr 9, 2021

Fix: cBioPortal/cbioportal#8437

  • Add splice in mutation type selector
  • Add "driver vs VUS" mutation type selector
  • Style changes to make it more compact
  • Hide "Other" if it's 0
With VUS Without VUS Previous
Mutation type selection panel image image image
Legend color codes image image image
Link Test select "Putative drivers vs VUS" Test deselect "Putative drivers vs VUS" Test

@leexgh
Copy link
Member Author

leexgh commented Apr 21, 2021

@jjgao @inodb Here are some mockup for driver annotation setting button, personally I like the first one most so add it to test link above, let me know which one do you prefer
1.
image

image

image

@inodb
Copy link
Member

inodb commented Apr 22, 2021

I like 2 :) - good to keep it consistent with the button at the top

@inodb
Copy link
Member

inodb commented Apr 22, 2021

Can we remove the "Other" Category when it is 0?

@inodb inodb requested a review from onursumer April 27, 2021 15:44
@leexgh leexgh changed the title Splice lollipop Add splice lollipop and driver annotation mutation type selector Apr 27, 2021
@leexgh leexgh force-pushed the splice-lollipop branch 2 times, most recently from c462979 to 98dee60 Compare April 27, 2021 21:56
@leexgh leexgh changed the title Add splice lollipop and driver annotation mutation type selector Add splice and driver annotation mutation type selector Apr 27, 2021
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! There seems to be a bug with the table search. Mutation type counts are not updated anymore after a search. Other than that the PR looks good to me.

'-'
);

const geneSummaryInfo = (
Copy link
Member

Choose a reason for hiding this comment

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

Should we add two functional components GeneSummaryInfo and CompactGeneSummaryInfo to keep the render function shorter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I move them outside render, but adding two functional components might introduce some duplicated code(some calculation code in both GeneSummaryInfo and CompactGeneSummaryInfo), so I just make them as one component

<div
data-test="germlineMutationRate"
className="invisible"
style={{ display: 'none' }}
Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember we didn't want { display: 'none' } because we wanted to render certain elements always at a fixed location. But now we are switching to a compact view, probably it's okay to do it this way.

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.

Looks good!

@inodb inodb merged commit 6069234 into cBioPortal:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants