-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Add Legend actions #20404
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
base: master
Are you sure you want to change the base?
[charts] Add Legend actions #20404
Conversation
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
I don't think so. Looks weird for now. Lets wait to see if there are real usecases
Doe sit impact a lot the technical solution? I don't think users will ask for it, so for now not having them looks ok |
It is a different approach, but it is not hard. It is just that right now they don't have any transition animation, so I wasn't sure it would make sense to add animations for these specific interactions. Maybe we keep it out for now and take a further look once we divide charts up in composable parts in the future |
| optionalDependencies: [ | ||
| // Optional as the series plugin starts first | ||
| UseChartVisibilityManagerSignature, | ||
| ]; |
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.
It's not really a dependency.
You have a dependency between plugins if the content of the plugins needs something from the other plugin.
For example the zoom plugin has a dependency on the brush one, because we use instance.setZoomBrushEnabled in the plugin.
If I get it correctly here you don't use it, the link between series and visibility plugins are only in the selectors
| /** | ||
| * Set of series ids that are currently hidden. | ||
| */ | ||
| hiddenIdentifiers: VisibilityItemIdentifier[]; |
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.
It feels weird to use an array, since what we want is a data structure to interrogate with "Is this item hidden?".
Could it be just an object { [id]: boolean }?
The only "issue" I see is the pie chart but we could consider that all slices should have unique id
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.
We can't really use seriesId alone. As you said, piechart uses the itemid as well.
An option is to concatenate those {seriesid}${itemid}, then we could simply have a Set<string> which would simplify some stuff.
Wdyt?
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.
My proposal was to avoid the concatenation on our side, and let it to the users.
A bit the same way we moved from "ids in xAxis must be different" to "ids in xAxis and yAxis ids must be different".
- If the user have a simple usecase (only one series, or already all item have unique id): Then no problem for them
- If they have multiple series with same item ids: They need to do the concatenation by themself.
It adds a bit of complexity for the second scenario, but:
- It feels like a good practice to concatenate nested ids to have them unique
- It does not look like a bug: When I ask to hide "id1", all item with
id: "Id1"are hidden - It's transparent for users: We don't try to outsmart them with some id concaenation expecting no collision
Plus that's a constraint we can be consistent with all charts. For example in sankey we can assume links and node don't have similar ids
| /** | ||
| * If true, the series will be hidden. | ||
| * @default false | ||
| */ | ||
| hidden?: boolean; |
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 it intended to let users provide hidden to the series?
If yes, this might be problematic when introducing control on the visibility. You will end-up with two source of information: series.hidden and visibility (or whatever name you give to the prop)
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 thought about using the series[].hidden/series[].data[].hidden props themselves. Though I guess updating series all the time might not be great 🤔
Having it accept an array of identifier objects could be an option. Internally we can move that to a concatenated string if we go with a Set as mentioned above.
// How do we hide the third slice of the second series of a pie chart?
// identifier obj
hidden=[
{
seriesId:'s2',
itemId: 'i3'
}
]
// string
hidden=[
's2:i3'
]
// series
series=[
{
hidden: true,
data: [
{},
{},
{ hidden: true },
]
}
]| if (hidden) { | ||
| return null; | ||
| } | ||
|
|
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.
Maybe a dumb idea: WHat about ignoring them directly in the useRadarSeriesData()
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
From the gifs, I can see that we are not changing the axis range based on hiding/viewing a series. We can keep it out of scope, until users request for it. Later on, a setting like Zoom filtermode: discard/keep, could be helpful for some use cases. |
Yeah, I intend to add it as a future iteration. Will create an issue so we don't forget 😆 |
Fixes #19455
Fixes #12344
Adds action to hide series/item when clicking on their legend.
Works by providing
toggleVisibilityOnClickto the chart component's slot props.Added a new
VisibilityManagerplugin.New Public API
Currently available for:
Pie
Scatter
Radar
Line
Bar
Open questions
Should missing charts types also have the behaviour?
VisibilityManagereventuallyShould we animate Scatter and Radar interaction?
Right now both don't have any animation, so I just don't render the component. If we want animations we will need to handle those.