Add user color configuration for oncoprinter #4817
Conversation
gblaih
force-pushed
the
oncoprinter-color-chooser
branch
from
6b2364d to
6c1631b
Compare
| @autobind | ||
| private getDefaultSelectedClinicalTrackColor(value: string) { | ||
| if (!this.selectedClinicalTrack) { | ||
| return [255, 255, 255, 1]; |
There was a problem hiding this comment.
make a constant out of this. its BLACK right?
| if (!this.selectedClinicalTrack) { | ||
| return [255, 255, 255, 1]; | ||
| } | ||
| if (this.selectedClinicalTrack.datatype === 'counts') { |
There was a problem hiding this comment.
use SWITCH statement for this instead of if/elseif
|
|
||
| @action.bound | ||
| setClinicalTrackColorChanged(changed: boolean) { | ||
| this.clinicalTrackColorChanged = changed; |
There was a problem hiding this comment.
seems odd to keep track of changed this way. i would expect this status to be derivable from the selection itself?
There was a problem hiding this comment.
i think you're right actually, we don't need this as the selection itself means the track changed.
| this._userSelectedClinicalTracksColors[label][value] = color; | ||
| } | ||
| localStorage.setItem( | ||
| 'oncoprinterClinicalTracksColorConfig', |
There was a problem hiding this comment.
make this a constant so it can be use to read it as well
| } | ||
|
|
||
| @autobind | ||
| private getDefaultSelectedClinicalTrackColor(value: string) { |
There was a problem hiding this comment.
maybe "getDefaultSelectedClinicalTrackColorForValue" just so it's clear that it's dependent on the value. Also, maybe change name of value param to attributeValue.
| case 'string': | ||
| // Mixed refers to when an event has multiple values (i.e. Sample Type for a patient event may have both Primary and Recurrence values) | ||
| if (value === 'Mixed') { | ||
| return [220, 57, 18, 1]; |
There was a problem hiding this comment.
make this a constant so we understand what it is.
| } else { | ||
| return this | ||
| .defaultClinicalAttributeColoringForStringDatatype[ | ||
| value |
There was a problem hiding this comment.
can you be sure that value will be present?
There was a problem hiding this comment.
it should always be present since this value comes from the track data which we used to obtain all the default colors.
| @@ -503,14 +521,28 @@ export function getClinicalTracks( | |||
| attr.countsCategories!.length === | |||
| MUTATION_SPECTRUM_FILLS.length | |||
| ) { | |||
| countsCategoryFills = MUTATION_SPECTRUM_FILLS; | |||
| countsCategoryFills = MUTATION_SPECTRUM_FILLS.slice(); | |||
| _.forEach(attr.countsCategories, (label, i) => { | |||
There was a problem hiding this comment.
i think this code would be clearer if you used map and conditional inside of the map wherein you return either the default (from MUTATION_SPECTRUM_FILLS) or userSelectedClinicalTracksColors entry. That brings the fundamental operation to the top (building countsCategoryFills)
| @@ -70,9 +73,24 @@ export default class OncoprinterStore { | |||
| @observable hideGermlineMutations = false; | |||
| @observable customDriverWarningHidden: boolean; | |||
|
|
|||
| @observable _userSelectedClinicalTracksColors: { | |||
| [label: string]: { | |||
There was a problem hiding this comment.
trackLabel and attributeValue will make this clearer
| const colorGetter = makeUniqueColorGetter( | ||
| _.values(RESERVED_CLINICAL_VALUE_COLORS) | ||
| ); | ||
| let categoryToColor = _.cloneDeep(RESERVED_CLINICAL_VALUE_COLORS); |
There was a problem hiding this comment.
i think you can avoid deep cloning here by instead merging RESERVED_CLINICAL_VALUE_COLORS onto a map made from data. This will be clearer also. That way RESERVED_CLINICAL_VALUE_COLORS will always win.
gblaih
force-pushed
the
oncoprinter-color-chooser
branch
from
f964259 to
70b4a78
Compare
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
gblaih
force-pushed
the
oncoprinter-color-chooser
branch
from
70b4a78 to
3efbff7
Compare
alisman
force-pushed
the
oncoprinter-color-chooser
branch
from
3efbff7 to
dbdbd4a
Compare
Implements cBioPortal/cbioportal#10502