[radar] Handling of Point Label Color as Array #5153
Conversation
Feature/color point labels
There was a problem hiding this comment.
One more general idea, we've used array parameters in other parts of the scales to set different options for each grid line.
What do you think about using a 2d array, for the font colour? That way the first level indexes into each point label and the 2nd level for each line. @simonbrunel thoughts on this idea? I fear it might be too confusing and hard to manage
src/scales/scale.radialLinear.js
Outdated
| } else { | ||
| pointLabelFontColor = valueOrDefault(pointLabelOpts.fontColor[x], globalDefaults.defaultFontColor); | ||
| } | ||
| var pointLabelPositionNew = JSON.parse(JSON.stringify(pointLabelPosition)); |
There was a problem hiding this comment.
[nit] if this needs to be cloned you could use helpers.clone() https://github.com/chartjs/Chart.js/blob/master/src/helpers/helpers.core.js#L156
src/scales/scale.radialLinear.js
Outdated
| if (pointLabelOpts.fontColor) { | ||
| // handle scale.pointLabels[i] as string or array combined with pointLabelFontColor as string or array or none given. | ||
| if (typeof scale.pointLabels[i] === 'string') { | ||
| if (typeof pointLabelOpts.fontColor === 'string') { |
There was a problem hiding this comment.
I would consider inverting this if and using helpers.isArray that way a fontColor that is not a string would still pass through (CanvasGradient, or CanvasPattern for example)
src/scales/scale.radialLinear.js
Outdated
| adjustPointPositionForLabelHeight(angle, scale._pointLabelSizes[i], pointLabelPositionNew); | ||
| ctx.fillStyle = pointLabelFontColor; | ||
| fillText(ctx, scale.pointLabels[i][x] || '', pointLabelPositionNew, plFont.size); | ||
| yOffset += plFont.size + 5; |
|
You right @etimberg, the first array level should be used for indexable options and that's actually a reasonable use case (a different color for each label). It's not implemented yet, but it needs to be consistent with the rest of the options API, especially when we will introduce scriptable options for the radar chart. A second array level seems complicated because it will always imply indexable options, meaning that you will have to duplicate your colors for each index: // first label 'blue', second 'red'
fontColor: ['blue', 'red']
// only the first label (first line 'blue', second 'red')
fontColor: [
['blue', 'red']
]
// first 4 labels
fontColor: [
['blue', 'red'],
['blue', 'red'],
['blue', 'red'],
['blue', 'red']
] At some point (for consistency), if we accept this feature, we will need to also support the same logic for the font size, weight, style, etc. but also in all places that accept multi-lines text. That will make the code really complicated (this PR already duplicates too much code for a very specific use case). |
Feature/color point labels
benmccann
changed the title
|
I think a 2-d array is too complicated. I think a better way to do that would be to support per-dataset options as is proposed in #5191 |
|
@benmccann I don't understand the "per-dataset options" relation? I think this PR is more comparable to supporting rich text, limited to a single use case: each line of the same text can have a different color. But I agree, the 2-d array is not a great approach and we may receive more and more requests to be able to draw a single text with different formatting: bold, italic, font size, etc., everywhere. Dealing with arrays is also too restrictive: how to handle the following case: "the first word of my one line text to be in bold or in a different color" (eg. "Foo: bar"). IMO, we should think about a more flexible approach, maybe some king of limited rich text language, and if it's too much code, expose a new hook and implement rich text in an external plugin. |
|
One index of the 2d array corresponds to the dataset. It would be possible to use only a 1d array and achieve the same results if the options were set on each dataset instead. |
|
Maybe it would make sense to implement this as is being done in https://github.com/chartjs/Chart.js/pull/5075/files#diff-76b3a981209f1918ec2431f4276b9703L270? That PR should be split up into two though instead of making two different changes in one PR |
|
I would rather not rely on arrays for that feature, per my previous comment. |
|
@JulianTrei I'm going to close this PR for now given that we've decided another approach would be best. Please feel free to reopen with the changes suggested. Maybe first it would be best to join the Slack channel and talk through any solution so that we're all on the same page regarding the approach and we don't waste any of your time. Thanks again for contributing |
Allows the input of Point Label Colors as array. This change makes it possible to color the axis labels accordingly to the dataset labels.