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 user color configuration for oncoprint #4699

Merged
merged 15 commits into from
Nov 20, 2023

Conversation

gblaih
Copy link
Contributor

@gblaih gblaih commented Aug 16, 2023

@gblaih gblaih marked this pull request as draft August 16, 2023 21:42
@gblaih gblaih force-pushed the oncoprint-color-configuration branch from 47130cf to c3551be Compare August 18, 2023 20:23
public setDefaultUserAlterationColors(
rule: IGeneticAlterationRuleSetParams
) {
for (let alteration in rule.rule_params.conditional) {
Copy link
Collaborator

@alisman alisman Aug 29, 2023

Choose a reason for hiding this comment

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

avoid using for loops. use functional approach instead. _.forIn(()=>{})
better separates the control logic (loop) from the business logic (the callback)

@@ -573,6 +574,17 @@ export class ResultsViewPageStore extends AnalysisStore

@observable queryFormVisible: boolean = false;

@observable userAlterationColors: {
[alteration: string]: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

alterationType

@@ -573,6 +574,17 @@ export class ResultsViewPageStore extends AnalysisStore

@observable queryFormVisible: boolean = false;

@observable userAlterationColors: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

userSelectedAlterationColors

@gblaih gblaih force-pushed the oncoprint-color-configuration branch 2 times, most recently from 1df8dcb to abdb06e Compare October 2, 2023 22:00
}

@action.bound
public setDefaultClinicalTrackColors(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't ever need to set the defaults. when the consumer of the track colors (the oncoprint rendring logic) requests a color, we look to see if there is a custom color for that track, and if not, we use a default. since that default is not static, we may need to "cache" the colors so that they are only generated once during a load of the page.

return colors;
}

buildColorChooserWidget = () => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

@computed get colorChooserElement() {

}

>
<span
style={{
float: 'right',
Copy link
Collaborator

Choose a reason for hiding this comment

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

we avoid using float css property. user flex box instead.

attribute.displayName
]
) {
oncoprint.props.store.setDefaultClinicalTrackColors(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should never set state from inside of an invoke (this is known as a side effect)

// set new color of value in track's category_to_color
// if undefined color, set value's color in track's category_to_color back to its default
if (color) {
(track as any).category_to_color[value] = color;
Copy link
Collaborator

Choose a reason for hiding this comment

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

look into whether we can avoid casting as any.

}
}

export function getClinicalTrackColor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be a method of the store

@@ -183,6 +250,12 @@ export default class ResultsViewOncoprint extends React.Component<
);
}

@computed get enableWhiteBackgroundForGlyphs() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

isEnabled...

@@ -253,6 +326,8 @@ export default class ResultsViewOncoprint extends React.Component<

@observable renderingComplete = false;

@observable changedTrackKey: string = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

trackKeySelectedForEdit

value: string,
color: RGBAColor | undefined
) {
setClinicalTrackColor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should not need to update two different things with essentially the same data. if we update
store, then the track should respond to the observable mutation. because of the particular nature of the oncopring library, this may be difficult.

}

@computed get selectedClinicalTrack() {
let index = _.findIndex(
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should just use _.find and deal with typescrupt in another fashion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

whoever is using this needs to be able to to handle an undefined case.

@gblaih gblaih force-pushed the oncoprint-color-configuration branch from 1abe082 to 48cd324 Compare November 6, 2023 16:38
};
};
} = JSON.parse(
localStorage.getItem('clinicalTracksColorConfig') || '{"global":{}}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

put localStorage get into constructor

@gblaih gblaih force-pushed the oncoprint-color-configuration branch 3 times, most recently from 9f682dc to 2fa7e98 Compare November 10, 2023 14:38
@gblaih gblaih marked this pull request as ready for review November 10, 2023 15:49
@gblaih gblaih force-pushed the oncoprint-color-configuration branch 5 times, most recently from 7c117ef to f7b7872 Compare November 17, 2023 17:34
@gblaih gblaih force-pushed the oncoprint-color-configuration branch from f7b7872 to 049fb6d Compare November 20, 2023 18:35
@alisman alisman merged commit 5281bbd into cBioPortal:master Nov 20, 2023
14 of 15 checks passed
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