Add a configurable legend to timeline tracks #4593
Conversation
alisman
commented
Apr 21, 2023
alisman
commented
|
@inodb Here is a track specific legend. To start with this must be configured in code on an instance basis. Since it seems people like to assign colors to timeline events in the data itself, we could create a dynamic legend based on whatever data was present. It might necessitate people putting redundant legend labels on every event. Re the order of attributes in tooltips, they are alphabetical. We stuck the dates at the bottom, but otherwise, we were sorting them. This PR removes that sorting which presumably would make the order reflect the original data files. However, I think @tmazor at one point requested that they be sorted alphabetically. So not sure what to do. We could make the order configurable in code, but we're stating to get pretty configuration heavy! Re the order of the tracks, I would suggest that we make the status tracks into a track group (would be done in data), to avoid them getting intermingled with other not status tracks. |
There was a problem hiding this comment.
Looks great!
Feedback:
- mousing over the tooltip makes the legend disappears
Here is a track specific legend. To start with this must be configured in code on an instance basis. Since it seems people like to assign colors to timeline events in the data itself, we could create a dynamic legend based on whatever data was present. It might necessitate people putting redundant legend labels on every event.
Yeah maybe we can stick with hardcoding this one legend for now for "Assessment" tracks exclusively. Haven't had requests about adding other legends yet
Re the order of attributes in tooltips, they are alphabetical. We stuck the dates at the bottom, but otherwise, we were sorting them. This PR removes that sorting which presumably would make the order reflect the original data files. However, I think @tmazor at one point requested that they be sorted alphabetically. So not sure what to do. We could make the order configurable in code, but we're stating to get pretty configuration heavy!
Yeah reflecting data order would be ideal I think. You get what you upload
Re the order of the tracks, I would suggest that we make the status tracks into a track group (would be done in data), to avoid them getting intermingled with other not status tracks
@ritikakundra is this feasible? Can we change the timeline files on our end somehow? Or can Sage point us to the scripts that do the data mangling so we can update it?
|
The legend is awesome! My only comment on that is the same as Ino's, that it keeps disappearing when I pull up a tooltip. In terms of the order of attributes, alphabetical order definitely sounds like something I would have requested. I still kinda prefer that, but I can also see the argument for just reflecting the data as it is provided and I'm ok with that. However, I think the reason we got on this topic now is that I think it would be clearer if the attribute that leads to the coloring were the first one in the tooltip, which is still not the case. So then is that something we can update in the data? |
alisman
force-pushed
the
bpcImprove
branch
3 times, most recently
from
1cf8a1c
to
95b2da1
Compare
| y={y} | ||
| height={track.height} | ||
| width={width} | ||
| ></TimelineTrack> |
There was a problem hiding this comment.
| ></TimelineTrack> | |
| /> |
| <TimelineTrackLegend | ||
| y={y + 20} | ||
| track={track.track} | ||
| ></TimelineTrackLegend> |
There was a problem hiding this comment.
| ></TimelineTrackLegend> | |
| /> |
| @@ -39,6 +46,11 @@ export enum TimelineTrackType { | |||
|
|
|||
| export type TimeLineColorGetter = (e: TimelineEvent) => string | void; | |||
|
|
|||
| export type LegendItem = { | |||
There was a problem hiding this comment.
Maybe name it TimelineLegendItem for consistency?
| @@ -24,6 +24,7 @@ import { ISampleMetaDeta } from 'pages/patientView/timeline/TimelineWrapper'; | |||
| import { ClinicalEvent } from 'cbioportal-ts-api-client'; | |||
| import { getColor } from 'cbioportal-frontend-commons'; | |||
| import ReactMarkdown from 'react-markdown'; | |||
| import { LegendItem } from 'cbioportal-clinical-timeline/src'; | |||
There was a problem hiding this comment.
This was also caught by ci/circleci: check_incorrect_import_statements
| import { LegendItem } from 'cbioportal-clinical-timeline/src'; | |
| import { LegendItem } from 'cbioportal-clinical-timeline'; |
alisman
force-pushed
the
bpcImprove
branch
3 times, most recently
from
40ce307
to
753645f
Compare
Remove sorting from event attributes in tooltip
