-
Notifications
You must be signed in to change notification settings - Fork 312
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 Stacked Bar Chart and sorting to Plots Tab #4960
base: master
Are you sure you want to change the base?
Add Stacked Bar Chart and sorting to Plots Tab #4960
Conversation
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@SURAJ-SHARMA27 this looks awesome - thank you! For the sorting, can we also sort by "# samples"? The value of the Y-axis basically This would be similar to e.g. how it works here for treatment response: This is slightly separate, but maybe for treatment response, the sorting should be using your new "Sort By" component too |
Looks great. It would be nice to also be able to sort by the overall number of samples (I see that comment above) and also to change the sort order back to alphabetical. |
Good work @SURAJ-SHARMA27 !
|
Actually, I implemented that earlier, as you can see in the video I attached at the time of the PR. The labels were changing, but I forgot to include the logic when pushing it. Thank you for pointing that out. I have now made the changes and pushed it. |
I think I have implemented most of the suggestions and added two more options for sorting: sort by the number of samples and alphabetically. If you could review it. @inodb screen-capture.14.webm |
Amazing - nice work @SURAJ-SHARMA27 !
@jjgao Good question - I kinda like it separate from x/y axis selection since we might also want to allow sorting by multiple values in the future (which could be like a multi-select dropdown element) Few more thoughts:
|
@@ -58,6 +58,11 @@ export interface IMultipleCategoryBarPlotProps { | |||
svgRef?: (svgContainer: SVGElement | null) => void; | |||
pValue: number | null; | |||
qValue: number | null; | |||
SortByDropDownOptions?: { value: string; label: 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.
sortByDropDownOptions
@@ -425,6 +430,43 @@ export default class MultipleCategoryBarPlot extends React.Component< | |||
|
|||
@computed get labels() { | |||
if (this.data.length > 0) { | |||
if (this.props.sortByOption == 'SortByTotalSum') { |
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.
===
); | ||
return sortedMajorCategories; | ||
} else if ( | ||
this.props.sortByOption != '' && |
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.
lets use === and !== throughout the code
this?.SortByDropDownOptions | ||
} | ||
updateDropDownOptions={ | ||
this?.updateDropDownOptions | ||
} | ||
sortByOption={this?.sortByOption} |
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.
is this?.
necessary? i'd think this.
is fine
if (this.props.sortByOption == 'SortByTotalSum') { | ||
const majorCategoryCounts: any = {}; | ||
|
||
this.data.forEach(item => { |
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 code looks pretty similar to the the function sortDataByOption
you made below. it would be nice to extract this into a function that can be reused
(a, b) => majorCategoryCounts[b] - majorCategoryCounts[a] | ||
); | ||
|
||
const reorderCounts = (counts: any) => { |
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.
instead of making this function inside, lets extract this into a new function outside for clarity and reuse
@SURAJ-SHARMA27 Thanks for the fixes! I noticed the "Sort by" option is currently missing here, do you see that? |
@@ -435,6 +451,18 @@ export default class MultipleCategoryBarPlot extends React.Component< | |||
} | |||
} | |||
|
|||
private setInitialSelectedOption = () => { |
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 is a bit of an anti-pattern. calling from the child component to update the state of parent component on render. it should be possible to do this in parent component prior to mount?
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.
In the parent component, it doesn't contain data in the form of IMultipleCategoryBarPlotData
, from which I have to extract all the unique major categories for dropdown options. Therefore, to avoid extra calculations, I passed the prop from the parent and updated it when it became available in the child.
sortByOption: string | undefined | ||
): string[] { | ||
if (sortByOption === 'SortByTotalSum') { | ||
const majorCategoryCounts: any = {}; |
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.
avoid 'any'. lets type this.
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.
Yes, I have defined the types across all the changes.
data: IMultipleCategoryBarPlotData[], | ||
sortByOption: string | undefined | ||
): string[] { | ||
if (sortByOption === 'SortByTotalSum') { |
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.
see if SortByTotalSum is available on an enum somewhere. seems likely that it is
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.
No, it was not there; I have defined an enumeration and used it
return Object.keys(majorCategoryCounts).sort( | ||
(a, b) => majorCategoryCounts[b] - majorCategoryCounts[a] | ||
); | ||
} else if (sortByOption !== '' && sortByOption !== 'alphabetically') { |
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.
use constant for 'alphabetically' or see if it's available on an enum
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.
I have defined an enumeration and used it
const sortedMajorCategories = getSortedMajorCategories(data, sortByOption); | ||
|
||
if (sortByOption === 'SortByTotalSum' || sortedMajorCategories.length > 0) { | ||
const reorderCounts = (counts: any) => { |
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.
avoid 'any'
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.
Yes, I have defined the types across all the changes.
@SURAJ-SHARMA27 correct
Thanks so much for fixing! |
okay sir, I think I did it correctly. You can review it (pushed already). The bars will be sorted according to the percentages of the selected minor category. If the number of samples is selected from the dropdown, the bars are sorted by the total absolute count for each stack. For alphabetical sorting, the bars will be sorted as usual. screen-capture.33.webm |
bc6e5b9
to
bbafcbf
Compare
bbafcbf
to
903134d
Compare
f7f261a
to
6abbaf5
Compare
Hi @SURAJ-SHARMA27, If you visit this link with the PR active, you'll see the following problem: If you are busy with school or work, we understand and will find someone else to look into it. |
Related ticket: cBioPortal/cbioportal#10722 |
@SURAJ-SHARMA27 just FYI, we have a dev @gblaih who is taking care of the last fixes. |
…centage bar charts
302c00b
to
a9f5d3d
Compare
Describe changes proposed in this pull request:
Addition of Sort By MinorCategory Options:
Sorted Stacks by Selected MinorCategory:
Any screenshots or GIFs?
screen-capture.9.webm
Notify reviewers
@alisman
@sowmiyaa-kumar
@zeynepkaragoz
@TJMKuijpers
@inodb