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

Implement gene set option in plots tab #1432

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

oplantalech
Copy link
Contributor

Add gene sets in the Plots tab. You test the functionality here: https://cbioportal-test.thehyve.net:8081/cbioportal/

Copy link
Contributor

@adamabeshouse adamabeshouse 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 besides this one possible issue, see my comment

@@ -918,7 +1006,7 @@ export default class PlotsTab extends React.Component<IPlotsTabProps,{}> {
/> Apply Log Scale
</label></div>
)}
<div className="form-group" style={{opacity:(axisSelection.dataType === CLIN_ATTR_DATA_TYPE ? 0 : 1)}}>
{(axisSelection.dataType !== CLIN_ATTR_DATA_TYPE && axisSelection.dataType !== GENESET_DATA_TYPE) && (<div className="form-group">
Copy link
Contributor

Choose a reason for hiding this comment

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

I was using opacity because that way the size of the panel doesn't change, which may be smoother UX. @alisman what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you prefer, I can keep the "gene" selection box hidden when selecting the clinical attribute instead of removing it to keep the size of the panel, as you implemented. Be aware though that now there are have four selection boxes (gene and gene set boxes are independent).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adamabeshouse I agree we want to avoid jumpiness. Oleg, I'm not sure how to ansewr question. I think I need to see it to undersatnd. Can you put screenshot or do demo for me?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oplantalech would there ever be the gene and geneset box visible at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamabeshouse No. Geneset box only appears when the geneset datatype is selected

Copy link
Contributor

Choose a reason for hiding this comment

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

@oplantalech would it be possible to reuse the same component, and just insert different options and a different label and a different onChange method?

Copy link
Contributor Author

@oplantalech oplantalech Sep 17, 2018

Choose a reason for hiding this comment

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

@adamabeshouse I guess so, but I think it is more logical to be a separate component. If you want to keep the same size for the selection boxes, I can keep the gene box in place without the option to see it when clinical attributes are selected (what you implemented). Will this solve your issue?

@@ -2454,6 +2467,10 @@ export class ResultsViewPageStore {
return new GeneCache();
}

@cached get genesetCache() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is this being used (the cache)? We are actually moving away from using caches like this so I wouldn't bother with this unless there's something i'm misunderstanding. We have implemented a client level cache (not merged yet) that will take care of this in a more abstract way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in line 1686 of this same file to retrieve the gene sets, similarly as done with genes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but where is it actually consumed by view layer? like where is cache data actually accessed?

// in that case theres no selected gene displayed so its confusing UX to have "Same gene" as an option
sameGenesetOption = [{ value: SAME_GENESET_OPTION_VALUE, label: `Same gene set (${this.horzSelection.selectedGenesetOption.label})`}];
}
return (sameGenesetOption || []).concat((this.horzGenesetOptions.result || []) as any[]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like should be typed with same as horzGenesetOptions.result

Copy link
Collaborator

Choose a reason for hiding this comment

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

i.e. not any[]

@@ -477,7 +477,7 @@ function makeAxisDataPromise_Geneset(
const value = d.value;
return {
uniqueSampleKey: d.uniqueSampleKey,
value
value: Number(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

wow good find :D

@oplantalech oplantalech changed the base branch from rc to master October 4, 2018 14:13
@alisman alisman merged commit 22770c9 into cBioPortal:master Oct 4, 2018
@oplantalech oplantalech deleted the gsva_plots branch October 5, 2018 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants