Annotation visualisation #1121
Annotation visualisation #1121
Conversation
* Update to API access request feature - Do not auto approve when academic user requested API access - Include justification in slack channel - Show API Access Justification in User page - Allow admin to change API access justification in User page * update screenshot tests
| selectedAnnotationColumns: string[]; | ||
| selectedTreatmentColumns: string[]; | ||
| } | ||
| export class AnnotationVisualisation extends React.Component< |
There was a problem hiding this comment.
Could you make this a function component. So just export function AnnotationVisualisation() { ... }
| 'selectedTreatmentColumns' | ||
| ); | ||
|
|
||
| if (savedAnnotationColumns && savedTreatmentColumns) { |
There was a problem hiding this comment.
Can only one of them be set? What happens in that case?
| return filteredColumns.map(column => ({ | ||
| ...getDefaultColumnDefinition( | ||
| column.key, | ||
| 1200 / this.state.selectedAnnotationColumns.length |
There was a problem hiding this comment.
I wouldn't just make every column have an even portion of the table width. Also you assume the table width is always 1200 I imagine. What about on changing screen sizes?
| ); | ||
| }; | ||
|
|
||
| export function getDefaultColumnDefinition<T>( |
There was a problem hiding this comment.
Not using template parameter
| ); | ||
| }; | ||
|
|
||
| handleTreatmentColumnsChange = (selectedOptions: any) => { |
There was a problem hiding this comment.
Can you use the type instead of any?
There was a problem hiding this comment.
This comment applies above as well and in most places you see any unless there is a good reason to use it
| ); | ||
| const totalColumns = selectedKeys.length; | ||
| const calculatedWidth = hasAnnotation | ||
| ? 800 / (totalColumns - 1) |
There was a problem hiding this comment.
Again, I'd be careful with these hardcoded widths. See my other comment for details.
| render() { | ||
| return ( | ||
| <> | ||
| {this.props.isPatientInfoVisible && ( |
There was a problem hiding this comment.
It looks like patientId and patientInfo are only used when isPatientInfoVisible === true. However, the first two props I mentioned are not optional. I would prefer to have these bundled into an optional object like the following:
patientInfo?: {
id: string
info: string
}
id and info could also be optional.
| </div> | ||
| </div> | ||
| )} | ||
| <Tabs |
There was a problem hiding this comment.
Just wondering why you didn't use the Tabs component we already had?
aishikanandi
force-pushed
the
vizualize-annotations
branch
from
ba957e8
to
e3987e2
Compare
This is the first draft of the visualisation component for OncoKB annotations.
To do: