-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
To test, just run |
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.
@jotak It looks veeery nice!
But there is an issue somewhere. Here are the issues I can see:
- When hiding a serie, the remaining series may change its colors. When you hover the remaining visible series, they are highlighted with the right color (so, you see a change in color). My guess is that colors seems to be assigned to series in order and colors are not skipped when a serie is disabled.
- After re-enabling a serie, of you hover on the legend the other series, you will notice that, now, highlighting has colors shifted (i.e. when you hover on the legend, the serie will change its color).
- Also, after a serie is re-enabled, highlighting on legend hover stops working for this lastly re-enabled serie.
You can see all this (shown in order) in this gif:
I'm guessing it's probably something related to array shifting :)
<ChartGroup offset={groupOffset}> | ||
{this.props.data.map((serie, idx) => { | ||
if (this.state.hiddenSeries.has(idx)) { | ||
return undefined; |
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.
Oh! I understand. I guess it's VictoryCharts who choose the line color given the themeColor={ChartThemeColor.multi}
This complicates things a little bit :/
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.
Perhaps a hack could be Opacity to 0%???
Good point, I didn't notice the first issue. As you say it's because of using theme colors. The 0% opacity hack would have a drawback: the y-domain is not going to be re-adjusted based on visible metrics, which is something I often find very useful (especially when you look at series that have different order of magnitude, hiding the "bigger" ones is often necessary when you need to have a closer look at the "smaller" ones, if you see what I mean. This is only possible with y-domain re-adjustment. |
@israel-hdez By the way, the other issue you are mentioning is something I've also noticed but unfortunately I fail to see what's causing this. I've been able to create a minimal reproducer and asked about that on victory forum but unfortunately no response yet :( |
@jotak It isn't very motivating to see that they haven't replied :( |
- Colors are used explictly now (yet still picked from themes) - Add tooltip as in Kiali sparklines - Add ability to provide a different theme
@israel-hdez yeah. As far as I see most contributions are from a single person / maintainer, so I'll wait a little bit and won't blame anybody, but yes i wished there was a bigger community involved there. |
btw, the other issue is addressed through last commit |
@israel-hdez do you mind take another look at? |
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.
Good!
Point (1) and (2) are fixed. Third one blocked on VictoryChars reply.
Nice work @jotak
Thanks! |
Closes kiali/kiali#1648
This is bringing into k-charted the same legend behaviours as what has been added in Kiali's sparklines / side panel bar charts