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

Oncoprint tracks menu: move groups and charts to top level #4368

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

gblaih
Copy link
Contributor

@gblaih gblaih commented Sep 7, 2022

!this.options.isComplete ||
this.options.result!.clinical.length === 0
!this.props.options.isComplete ||
this.props.options.result!.clinical.length === 0
}
>
{this.addClinicalTracksMenu.component}
</MSKTab>
<MSKTab
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this is what I was looking for. There is still a possibility that there will be more than one tab because of the custom charts tab. Thus, we need to keep the menu. I see that we are already hiding the menu when there is only one option, so it seems like no change is necessary here. (If there was only ever a single tab, it would make sense to just get rid of the tab structure altogether).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gblaih @inodb now that I think about it, I don't think the custom charts belongs in the clinical tab submenu either. so, really we should have 4 top level tabs and NO subtabs ever.

showLastRenderWhenPending: true,
});

readonly options = remoteData({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this name could be improved (i realize it was existing). It's really track options by type. "trackOptionsByType" ?

onAddAll: (keys: string[]) => void;
onClearAll: (keys: string[]) => void;
onToggleOption: (key: string) => void;
selectedClinicalAttributeIds: Dictionary<string>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

see if you can use the Typescript Record type instead of this lodash supplied type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i mean, continue to use _.keyBy, that's correct, but see if you change the typing of this propery to Record if it will work. Record is generic so you'll have to supply the key and value types, e.g. Record<string,string>

@alisman
Copy link
Collaborator

alisman commented Sep 7, 2022

@gblaih we need to fix the following, where there is now only one subtab tab visible in clinical tab, we shouldn't show the sub tabs at all. that logic was actually working before, but something about changes seems to have broken it.

image

@gblaih gblaih force-pushed the groups-top-level-tab branch 2 times, most recently from 188e36b to a05d8da Compare September 8, 2022 17:34
@gblaih gblaih force-pushed the groups-top-level-tab branch from a05d8da to c8da21c Compare September 8, 2022 17:51
@alisman alisman changed the title moved groups subtab to top level tab Oncoprint tracks menu: move groups and charts to top level Sep 8, 2022
@alisman alisman merged commit e2c6b9c into cBioPortal:master Sep 8, 2022
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.

2 participants