-
Notifications
You must be signed in to change notification settings - Fork 304
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 higher granularity CNA charts on Study View #4706
Add higher granularity CNA charts on Study View #4706
Conversation
fixing additional field in DataFilterValue
f318eb2
to
0665599
Compare
packages/cbioportal-frontend-commons/src/lib/AlterationColors.ts
Outdated
Show resolved
Hide resolved
@@ -204,6 +213,20 @@ export class StudySummaryTab extends React.Component< | |||
props.promise = this.store.getGenericAssayChartDataCount( | |||
chartMeta | |||
); | |||
} else if ( |
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.
weird that gene specific chart should be exclusive with, say "generic assay chart". Perhaps it's just semantics but we should improve the name then.
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.
We have another if
block for generic assay chart
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.
how much effort would it take to refactor/break up this switch statement (which then has the internal if else statement. it's just way too long and complicated.
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.
This will take time as it impacts all type of charts. I prefer to it as part of another PR.
@@ -5816,12 +5933,12 @@ export class StudyViewPageStore | |||
) | |||
.uniq() | |||
.value(); | |||
if (newChart.datatype === 'STRING') { | |||
if (newChart.datatype === DataType.STRING) { |
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.
👍
@kalletlak Found one little issue with mapping CNA to label values. Probably painful : ( |
It is resolved now |
@@ -325,6 +348,20 @@ export class StudySummaryTab extends React.Component< | |||
props.promise = this.store.getGenericAssayChartDataCount( | |||
chartMeta | |||
); | |||
} else if ( |
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.
when a chart type has subtypes (requiring these if/else statements), we should detect subtype here and then pass to helper function, which will use another switch statement or lookup table
{
[chartSubType]: (...)=>props
}
Fix cBioPortal/cbioportal#10257 and part of cBioPortal/cbioportal#6759