Add borderDash options to arc element#11127
Conversation
| return; | ||
| } | ||
|
|
||
| ctx.setLineDash(borderDash || []); |
There was a problem hiding this comment.
You set the defat to an empty array so this check is not necessary then right?
There was a problem hiding this comment.
Yes, that was my doubt as well. But I have seen in the other parts of code, everytime this check is done for borderDash because setLineDash fails if not an array, I guess. Therefore not clear to me if it's not correct on the other parts or if better to leave this check anyway :(
There was a problem hiding this comment.
@LeeLenaleee don't ask me why because I don't have the answer now (I checked this morning without success) but it seems that the donut options doesn't fallback to element ones therefore if you set borderDash: null in dataset config (and you don't have those check), you have an issue. This weird behavior was already recognized I guess because the _indexable about spacing option was added to the controller instead of the element. If you agree, I can pend more time to go in deep why the donut doesn't fallback to the arc element (maybe in another PR)
There was a problem hiding this comment.
@LeeLenaleee I have checked more in deep and the descriptors are working well. I wasn't aware that the resolver uses the descriptors of the default of the controller (and not in cascade as I was thinking) therefore it's working as designed.
About the point of your review, I can confirm that if you set borderDash: null, you have an error (but fallback works correctly if you set to undefined).
The check where null is passed as defined is here:
Chart.js/src/helpers/helpers.config.js
Line 331 in e417c60
The define function checks if the value is undefined but with null argument returns true. Probably the define function could be remove, using instead isNullOrUndef. WDYT?
In my opinion, but I can be wrong, it would be better to maintain that check anyway in the meanwhile. But feel free to disagree ;)
There was a problem hiding this comment.
Null is considered a valid value for configuration while undefined is considered not defined and thus fallback. Falling back from null would be a breaking change, extending to all unknown use cases.
There was a problem hiding this comment.
@kurkle yes!
EDIT: this is why I would suggest to stay as is, in this PR .
There was a problem hiding this comment.
I kind of agree with @LeeLenaleee, but the behaviour should be changed everywhere to be consistent, so maybe do at v5 as a breaking change.
There was a problem hiding this comment.
@kurkle @LeeLenaleee @etimberg I think we agreed to change for v5. Nevertheless let me outline that there are other options/situation which should be addressed, similar to this one, I guess. Maybe you are already aware but let me make an example.
The doughnut controller is setting spacing options in the default, set to 0. doing that, the spacing option of the element is ignored, therefore having a following chart config
{
type: 'doughnut',
data: {
labels: [0, 1, 2, 3, 4, 5],
datasets: [
{
data: [0, 2, 4, null, 6, 8],
cutout: 200
}]
},
options: {
elements: {
arc: {
spacing: 30
}
}
}
}the spacing: 30 is ignored in the cutout/maxsize calculation because it uses 0 (controller defaults) instead of 30. This is happening because the dataset options are not using the fallback in the update phase of the controller and when you have to use options of data element during the update of the controller, you must check against a default or replicate the options definition, ignoring the element config value/defaults. @kurkle I think we have already talked about that in the treemap controller.
| static descriptors = { | ||
| _scriptable: (name) => name !== 'spacing', | ||
| _indexable: (name) => name !== 'spacing', | ||
| _indexable: (name) => name !== 'spacing' && !name.startsWith('borderDash') && !name.startsWith('hoverBorderDash'), |
There was a problem hiding this comment.
why startsWith? and probably does not need hoverBorderDash, as its returned as borderDash when hovered.
There was a problem hiding this comment.
@kurkle I used startsWith to manage also borderDashOffset at the say way of borderDah because the 2 options are strictly related.
There was a problem hiding this comment.
probably does not need
hoverBorderDash, as its returned asborderDashwhen hovered.
It seems you're right. I was convinced that the indexable policy is ignored for hover if not specified but it doesn't seem so. I'll submit another PR to remove it.
| return; | ||
| } | ||
|
|
||
| ctx.setLineDash(borderDash || []); |
There was a problem hiding this comment.
I kind of agree with @LeeLenaleee, but the behaviour should be changed everywhere to be consistent, so maybe do at v5 as a breaking change.
4 participants
Fix #11126