-
Notifications
You must be signed in to change notification settings - Fork 313
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
Gsoc 2024 single cell tab- stackedBar,piechart and bar plots added #4921
base: master
Are you sure you want to change the base?
Gsoc 2024 single cell tab- stackedBar,piechart and bar plots added #4921
Conversation
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Hi Suraj, I left some comments to your code. Please have a look and let me know if something is not clear.
interface Entity { | ||
stableId: string; | ||
} | ||
interface DataBin { |
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.
DataBin is defined as an export type in StudyViewUtils.tsx
, you should import it from there.
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.
done
@@ -158,6 +158,7 @@ export async function fetchGenericAssayData( | |||
} as GenericAssayFilter, | |||
}; | |||
}); | |||
console.log(params, 'here are params2'); |
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.
Please remove the console.log().
ref={(ref: any) => (this.svg = ref)} | ||
<> | ||
{console.log(this.props.onUserSelection)} | ||
<DefaultTooltip |
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.
Please remove the console.log()
@@ -265,6 +265,7 @@ export default class PieChart extends React.Component<IPieChartProps, {}> | |||
)})`, | |||
})); | |||
const colorScale = this.props.data.map(data => data.color); | |||
console.log(colorScale, 'colorScale1'); |
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.
Please remove the console.log()
@@ -1395,6 +1396,12 @@ export class StudyViewPageStore | |||
const chartInfo = this._genericAssayChartMap.get( | |||
chartMeta.uniqueKey | |||
)!; | |||
console.log( |
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.
Please remove console.log()
// Create a ref to hold the SVG container | ||
const svgRef = useRef<SVGSVGElement>(null); | ||
// Set to store unique patient IDs | ||
let differentPatientIds: 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.
Try to avoid let
and use const
(also for the other variables defined below)
src/pages/SingleCell/PieToolTip.tsx
Outdated
}, [isHovered, tooltipHovered, tooltipEnabled]); | ||
|
||
// Define color scale (replace with your desired colors) | ||
const colors = [ |
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 think you are defining the same colors in multiple files. You could create a file SingeCellPageUtils
, define the colors there and import them when needed.
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 colors in singleCellStore and used them by importing from there.
src/pages/SingleCell/PieToolTip.tsx
Outdated
}; | ||
}); | ||
|
||
const handleDownloadData = () => { |
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.
Why do we need a handleDownloadData
in the tooltip? Is it possible to only download the tooltip? And like mentioned before, try to generalize this handleDownloadData
, place it in downloadUtils
and import it.
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.
And please explain why we cannot use the existing download function of cBioPortal
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 generalized all the download functions into downloadUtils. The reason I haven't reused the existing download functions is that we need additional customization. For instance, in the histogram and pie chart, we want the downloaded chart to include percentages next to the legends, even though these percentages are not displayed in the chart. Because of these customizations, I wrote specific functions.
src/pages/SingleCell/PieToolTip.tsx
Outdated
document.body.removeChild(link); | ||
URL.revokeObjectURL(url); | ||
}; | ||
const handleDownload = () => { |
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.
Same comment as handleDownloadData
above
Fix cBioPortal/cbioportal# (see https://help.github.com/en/articles/closing-issues-using-keywords)
Describe changes proposed in this pull request:
Any screenshots or GIFs?
screen-capture.13.webm
Notify reviewers
@zeynepkaragoz
@sowmiyaa-kumar
@alisman