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

Enable centered label drawing for doughnut controllers #825

Merged
merged 42 commits into from
Oct 10, 2024

Conversation

stockiNail
Copy link
Collaborator

@stockiNail stockiNail commented Jan 2, 2023

A prototype.
This PR is enabling the possibility to add an inner label in the middle of a doughnut controller.
This is leveraging on the already label calculation and drawing used for label annotation.

image text
inner2 inner1
inner4 inner3

SAMPLE chart configuration:

{
  type: 'doughnut',
  data: {
    labels: ['Data1', 'Data2', 'Data3', 'Data4'],
    datasets: [ {
      data: [102, 200, 80, 55],
    }],
  },
  options: {
    aspectRatio: 2,
    plugins: {
      annotation: {
        annotations: {
          innerlabel: {
            type: 'doughnutLabel',
            display: true,
            drawTime: 'afterDraw',
            font: [{size: 24},{size: 16, weight: 'bold'}],
            click(context) {
              console.log("click", context);
            },
            color: ['blue', 'red'],
            content: ({chart}) => ['Utilization', chart.data.datasets[0].data.length + '%'],
          }
        }
      }
    }
  }
}

TODO

@stockiNail
Copy link
Collaborator Author

I don”t think the innerlabel is needed. All can be mamaged by annotation plugin. New updates asap

src/elements.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Jan 7, 2023

About the animations, without reading any code;

The element and its options were designed to be the "view", containing currently displayed value for everything.

I'm guessing you are using an array of colors for the lines of text?

In this "philosophy", each line should be an "element", containing its resolved current display values.

I put quotation marks around the element, because from the animations point of view, it can be any object.

Now, I'm out of context wit the annotations plugin, but I think something like sub elements is a thing or was it abandoned?

@stockiNail
Copy link
Collaborator Author

I'm guessing you are using an array of colors for the lines of text?

yes, that's correct

In this "philosophy", each line should be an "element", containing its resolved current display values.

I understand the philosophy and I agree. But a value is a value, also an array could be a value ;), therefore the implementation it should not be against of that philosophy, I guess.
About the plugin implementation, to have an element for each row of a label when a content is a text, sounds to me adding complexity to manage events, options (like padding, borders, etc) which I don't think make sense to address because a complete refactoring should be needed with a few vantages. Furthermore I'm expecting that also the performance will be affected, having potentially many elements to manage for each row.

I put quotation marks around the element, because from the animations point of view, it can be any object.

I have to admit that this use case forced me to go more in deep how animation is working and I have learned more, that's good! Nevertheless, staying on this use case and the plugin itself, the animation was thought (at least for what I had understood) only for the properties of the elements and not for their options. In fact we have only 1 animations configuration, based on the properties of the elements. And, in my opinion, doesn't make sense to extend it to the options (like color, bordeColor, etc.). And this animations setup (for colors) is coming from the controller which is implementing that, and plugin animations config is falling back to that.
Maybe you disagree, but I think that assign the option to the element after animation item creation sounds good even if not 100% align with the designed element model and its philosophy. What do you think?
I would suggest to stay with this implementation, simple, fitting the requirement and working without big effort.

I think something like sub elements is a thing or was it abandoned?

It's still present for all labels and the points (for polygons). Nothing is changed. But, as written above, it doesn't seem to make sense to have sub-elements for each row. I think good compromise is what I described above, assign options to element after animation item creation.

Now, I'm out of context wit the annotations plugin,

Ok... may I ask what it means exactly? is it just a period and it will be forever? I'm worried that you will leave the project... 😟

@kurkle
Copy link
Member

kurkle commented Jan 7, 2023

I understand the philosophy and I agree. But a value is a value, also an array could be a value ;), therefore the implementation it should not be against of that philosophy, I guess. About the plugin implementation, to have an element for each row of a label when a content is a text, sounds to me adding complexity to manage events, options (like padding, borders, etc) which I don't think make sense to address because a complete refactoring should be needed with a few vantages. Furthermore I'm expecting that also the performance will be affected, having potentially many elements to manage for each row.

Yes, it might not be appropriate. Though then you could have hover / click effects for each row etc :)
Anyway, array is also a value, it just can't be animated. It should be possible to provide animation defaults for the element to disable animation of the options that should not be animated. (still did not look at the code, sorry :))

Another option could be to use something else, like textColor for that, which would not be animated by default.

Now, I'm out of context wit the annotations plugin,

Ok... may I ask what it means exactly? is it just a period and it will be forever? I'm worried that you will leave the project... 😟

It jus means its been long enough that I don't remember how things are anymore (hence the question about sub elements).
I still did not have the time to invest in these projects, sorry about that. Does not look good for today either :/.

@stockiNail
Copy link
Collaborator Author

It should be possible to provide animation defaults for the element to disable animation of the options that should not be animated.

I put it in my TODO. I will have a look in next days. I think it could make sense, only for color option.

Another option could be to use something else, like textColor for that, which would not be animated by default.

Personally, I don't like it because it's a deviation of the other plugins and chart.js itself where they are using color for text color. And a breaking change as well.

It jus means its been long enough that I don't remember how things are anymore (hence the question about sub elements).
I still did not have the time to invest in these projects, sorry about that. Does not look good for today either :/.

Fiuuuu... 😄 ... I had some time during the Christmas period but starting from next Monday, I will not have much time as well.

@stockiNail
Copy link
Collaborator Author

Anyway, I'm still working on this PR and I'd like to have something really consistent to show you asap (apart the discussion about animation). I have refactored it during last 2 days to provide the background inside the doughnut controller (and not the box around the label which doesn't make sense).

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

some quick comments, but this is not a proper review.

src/elements.js Outdated Show resolved Hide resolved
src/elements.js Outdated Show resolved Hide resolved
src/types/doughnutLabel.js Outdated Show resolved Hide resolved
kurkle
kurkle previously approved these changes May 9, 2023
Conflicts:
	src/helpers/helpers.options.js
	types/options.d.ts
@stockiNail
Copy link
Collaborator Author

@kurkle apologize because your approval was dismissed. PR had to be rebased

Conflicts:
	README.md
	docs/.vuepress/config.js
	docs/guide/index.md
@mesqueeb
Copy link

@kurkle what would be the steps needed to get this merged? 🤩

@stockiNail
Copy link
Collaborator Author

@LeeLenaleee I have rebased. Ready for review, when you have time.

LeeLenaleee
LeeLenaleee previously approved these changes Oct 6, 2024
docs/.vuepress/config.js Outdated Show resolved Hide resolved
@stockiNail stockiNail merged commit 938ef5c into chartjs:master Oct 10, 2024
10 checks passed
@stockiNail stockiNail deleted the innerLabelProto branch October 10, 2024 12:48
@mesqueeb
Copy link

Wow! Are donut center labels now supported natively? 🤩

@LeeLenaleee
Copy link
Collaborator

Not yet, there is no version released yet, I have made a bump pr so we can release one:
#951

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.

5 participants