Skip to content
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

Use global element hidden status for Pie charts #7156

Merged
merged 2 commits into from
Mar 1, 2020

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Feb 25, 2020

Fixes: #6746

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to store something on the chart for this?

@kurkle
Copy link
Member Author

kurkle commented Feb 26, 2020

Is it a good idea to store something on the chart for this?

Seemed like a good holder, but maybe not. I pushed an alternative solution to review.

@benmccann
Copy link
Contributor

What's expected when you update the chart? Should it clear the hidden elements? E.g. you have a completely new dataset, I'm guessing you might not want to hide based on index of the previously clicked dataset

@kurkle
Copy link
Member Author

kurkle commented Feb 26, 2020

What's expected when you update the chart? Should it clear the hidden elements? E.g. you have a completely new dataset, I'm guessing you might not want to hide based on index of the previously clicked dataset

As in the linked issue, when you hide an item and then add another dataset, the legend shows that the item is hidden, but its only hidden from the old dataset(s). When you click the legend again, the old datasets show that index and the new dataset hides that index. I don't think anyone expects that kind of behavior.

So this pr: the new dataset's item is also hidden = in sync with the legend.

This does not work with custom legend implementation though, because there is no api.

@kurkle kurkle force-pushed the hidden-pie branch 2 times, most recently from d177b53 to 6bd0da6 Compare February 27, 2020 07:36
etimberg
etimberg previously approved these changes Feb 27, 2020
@benmccann
Copy link
Contributor

It looks like DatasetController.getMinMax used hidden. You might have to override it in the doughnut / polarArea controllers? I'm also wondering if we still need hidden in core.element after this?

@kurkle
Copy link
Member Author

kurkle commented Feb 28, 2020

It looks like DatasetController.getMinMax used hidden. You might have to override it in the doughnut / polarArea controllers? I'm also wondering if we still need hidden in core.element after this?

I went ahead and removed the hidden

@benmccann
Copy link
Contributor

I'm very happy to have hidden removed!

Can we move toggleDataVisibility, getDataVisibility, and _hiddenIndices from core.controller.js to the doughnut and polarArea controllers? It seems quite specific to those charts, so I'm not sure I want to expose it globally when it will have no effect on the other chart types

Also, I wonder if there's a way we can share the legend settings between the doughnut and polarArea charts? (there's one extra if check in the doughnut onClick, so we'd have to make sure to use the doughnut version of the code so as not to accidentally lose that extra check)

@kurkle
Copy link
Member Author

kurkle commented Feb 28, 2020

It would actually work for others too, it does not care what indices are used. So could as well be datasetIndex. Could remove the hidden from dataset too. How does that sound?

@benmccann
Copy link
Contributor

Ok. I guess that would be fine

@kurkle
Copy link
Member Author

kurkle commented Feb 29, 2020

It would actually work for others too, it does not care what indices are used. So could as well be datasetIndex. Could remove the hidden from dataset too. How does that sound?

Ok. I guess that would be fine

I'm not going to do that though.

@etimberg etimberg merged commit a3dddb4 into chartjs:master Mar 1, 2020
@etimberg etimberg added this to the Version 3.0 milestone Mar 1, 2020
@kurkle kurkle deleted the hidden-pie branch June 12, 2020 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hidden data not applied on Pie chart when new dataset is added
3 participants