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

Fix the missing apply for the color on the legend labels #10855

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

stockiNail
Copy link
Contributor

@stockiNail stockiNail commented Nov 4, 2022

Fix #10854

@LeeLenaleee
Copy link
Collaborator

I gave the doughnut controller a quick look, there they override the generate labels and don't set the font color, if we do that I'm pretty sure it also should work and it would be the nicer solution in my oppinion

@stockiNail
Copy link
Contributor Author

I gave the doughnut controller a quick look, there they override the generate labels and don't set the font color, if we do that I'm pretty sure it also should work and it would be the nicer solution in my oppinion

Ah ok, I understood better now that PR. Therefore we should change the generateLabels adding the font color! Therefore we must change doughnut and polarArea generateLabels in the controllers

@stockiNail stockiNail marked this pull request as draft November 4, 2022 16:06
@stockiNail stockiNail marked this pull request as ready for review November 4, 2022 16:27
@stockiNail
Copy link
Contributor Author

stockiNail commented Nov 4, 2022

@LeeLenaleee I have changed PR as you suggested.

EDIT: about CC, the generateLabels functions in polar and doughnut controllers are equals! But it was previously as is as well.

@stockiNail
Copy link
Contributor Author

stockiNail commented Nov 4, 2022

@LeeLenaleee there is a side effect. generateLabels is a public API and whoever implement it must apply the color of legend labels otherwise it will be ignored I guess

@LeeLenaleee
Copy link
Collaborator

That is true but the same goes for the other options like providing the extra data so the default onClick keeps working.

So I am fine with that

@stockiNail
Copy link
Contributor Author

Yes but for this use case you have the same situation for pointStyle and only for it and probably the pointStyle case is managed by plugin. I will have a look later before going on, if you agree

@stockiNail
Copy link
Contributor Author

@LeeLenaleee I had a look and I see that it's working as you correctly reported. Therefore it's fine for me as well. Last remark. I had the feeling this is a breaking change for whom developed own generateLabels callback. Should this PR be tagged as breaking change and report it in the migration guide?

@LeeLenaleee
Copy link
Collaborator

I guess you could do it, technically the or you mentioned that introduced this got tagged breaking and put a note in migration guide but that is probably not something people will associate with this.

So many you could extend that migration note and keep this as a bugfix

@LeeLenaleee LeeLenaleee added this to the Version 4.0 milestone Nov 4, 2022
@stockiNail
Copy link
Contributor Author

Apologize... I think I'm tired or getting old and I haven't seen the note about fontColor.... 100% with you! Let's stay as is for all! Thank you!

@LeeLenaleee
Copy link
Collaborator

Np, np, no need to apologize for those kind of things or invalidating approval because you fixed cc or something else. Those things are part of the job and happen to all of us, makes us human :)

@etimberg etimberg merged commit 5a90b71 into chartjs:master Nov 4, 2022
@dargmuesli
Copy link

Thank you for the quick and kind fix! ❤️ If you can spend one, a new alpha release would be fancy 🤓

@stockiNail stockiNail deleted the fixLegendLabelsColor branch November 5, 2022 17:11
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.

Legend label color does not change in v4
4 participants