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

Hiding annotations along with datasets #244

Closed
joshkel opened this issue Oct 28, 2020 · 6 comments · Fixed by #246
Closed

Hiding annotations along with datasets #244

joshkel opened this issue Oct 28, 2020 · 6 comments · Fixed by #246

Comments

@joshkel
Copy link
Contributor

joshkel commented Oct 28, 2020

I have some annotations that are closely tied with particular datasets, so I'd like to hide them whenever the user hides the corresponding dataset by clicking on the dataset's legend item.

I can think of a couple of possible ways of doing this:

  1. Use a legend onClick handler (similar to Chart.js's multi series pie example) to programmatically show and hide the annotation items. Unfortunately, there's currently no direct way of doing that (Cannot disable an annotation box / line #221). (Temporarily setting the colors to transparent then restoring them when done would probably work, but it's awkward.)
  2. Give users the ability to specify an optional datasetIndex property on each annotation object (similar to the datasetIndex properties that Chart.js maintains for its legend items and tooltip items); then chartjs-plugin-annotation can use that to check if the corresponding dataset is visible and skip it if not.

Would you be interested in a PR implementing either a direct way to hide annotations (via an enabled or display or hidden boolean property) or implementing the datasetIndex approach? The datasetIndex approach makes a lot of sense to me, but if that's too special-purpose to want to support in chartjs-plugin-annotation, then that's fine.

Thank you.

@kurkle
Copy link
Member

kurkle commented Oct 28, 2020

I think datasetIndex and/or dataIndex binding would be required in any case, so that makes sense. I think there is a relatively new api in core to check for the visibility too. PR's are welcomed :)

@stockiNail
Copy link
Collaborator

@joshkel @kurkle I have some doubts about the proposed solution based on new property datasetIndex.

The dataset index can change during the lyfe cycle of a chart (a dataset can be removed and add new dataset at the same index of the array). This could create an inconsistent behavior.

The current CHART.JS implementation doesn 't have any configurable unique id at dataset level because the datasets are managed in a array. CHART.JS should change the dataset property in order to have an object (where every property contains a dataset) like has been done for the scales, in version 3. But in my opinion it's a disruptive change that in my opinion does not make sense.

You should add a custom property into the dataset (like id) and also into the plugin configuration and teach the plugin to use that property.

Nevertheless, like other plugins, see Datalabels, you should configure the plugin at dataset level. In this way, you don't need to configure any index or key. In this way, the annotation can work strictly to the life cycle of the dataset.
That's for me the preferred solution.

About enabled or display or hidden, the annotation plugin is related to axes (not datasets) and the axes are using display in order do not show them.
Therefore in my opinion display should be implemented in order to hide the annotation (by the configuration or programmatically).

@kurkle
Copy link
Member

kurkle commented Oct 29, 2020

Good points @stockiNail. I'm not sure if dataset level config works currently, but that would seem like a good approach.

@stockiNail
Copy link
Collaborator

@kurkle No, it doesn't, at least when I had a look to the plugin (I have re-written it in java for my project), 1 month ago.

Anyway, another change to do, before addressing the plugin options at dataset level, it's to remove the usage of array into configuration (see chartjs/Chart.js#7754) because the configuration can not be merged correctly.

In my java implementation I have removed the array going to a object and every annotation has got own id (like the scales in CHART.JS version 3). Having that, you should think to start implementing the configuration at dataset level because in this way you can integrate the configuration of the other levels.

Maybe that's not the right issue (I'm gonna open some new ones, when I have more time than now) about some missing capabilities that I implemented in my project:

  • callback to calculate the values of annotation at runtime. This could open the door to a simple implementation of some statistics, like average, standard deviation, linear regression, and so on. To do that, we should also change the configuration, where the values are properties of a specific object (to improve performance about the amount of callbacks call), like it's done in Zoom, with rangeMin and rangeMax. Here could be different but the same concept. In this plugin could be xValue, yValue (box) and value (line).

  • new property, something like autoRotation, where the rotation of the label is automatically calculated if we don't a straight line, but an oblique one. The linear equation function of the plugin can easily provide the angle of the line. This is because you can not now the degrees of the line and the rotation currently is useful if you want to set the standard rotation (45, 90, 180, 360).

I didn't raise any PR or issue, because the project looked like unmaintained

@stockiNail
Copy link
Collaborator

stockiNail commented Oct 30, 2020

@kurkle I am gonna submit a PR about this issue:

  • Adding display property to annotation
  • Enabling the property with a callback too, providing the option to show or hide the annotation based on user logic, for instance if a dataset is hidden then the annotation is not shown

Make sense?

@stockiNail
Copy link
Collaborator

@kurkle @joshkel PR #246 has been submitted.

I found an issue on the signature of the CHART.JS plugin interface methods (beforeUdate and afterUpdate) because they have been written with args argument which it has been introduce later that beta-4. Maybe I'm wrong.
I didn't change any doc (readme sounds related to previous version) and I don't think it should be added to migration one. Let me know.

As usual, I'm not a javascript coder, therefore feel free to help me to improve that code is needed.

kurkle pushed a commit that referenced this issue Oct 30, 2020
* Fixes #244

* "Expected method shorthand" issue solved

* Removes extra whitespace, useless function and changes order of checking

* Removes withespaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants