Add borderDash options to arc element #11127
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,13 +213,16 @@ function drawBorder( | |
| circular: boolean, | ||
| ) { | ||
| const {fullCircles, startAngle, circumference, options} = element; | ||
| const {borderWidth, borderJoinStyle, borderDash, borderDashOffset} = options; | ||
| const inner = options.borderAlign === 'inner'; | ||
|
|
||
| if (!borderWidth) { | ||
| 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 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 Chart.js/src/helpers/helpers.config.js Line 331 in e417c60 The 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 {
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 |
||
| ctx.lineDashOffset = borderDashOffset; | ||
|
|
||
| if (inner) { | ||
| ctx.lineWidth = borderWidth * 2; | ||
| ctx.lineJoin = borderJoinStyle || 'round'; | ||
|
|
@@ -264,6 +267,8 @@ export default class ArcElement extends Element<ArcProps, ArcOptions> { | |
| static defaults = { | ||
| borderAlign: 'center', | ||
| borderColor: '#fff', | ||
| borderDash: [], | ||
| borderDashOffset: 0, | ||
| borderJoinStyle: undefined, | ||
| borderRadius: 0, | ||
| borderWidth: 2, | ||
|
|
@@ -277,6 +282,11 @@ export default class ArcElement extends Element<ArcProps, ArcOptions> { | |
| backgroundColor: 'backgroundColor' | ||
| }; | ||
|
|
||
| static descriptors = { | ||
| _scriptable: true, | ||
| _indexable: (name) => name !== 'borderDash' | ||
| }; | ||
|
|
||
| circumference: number; | ||
| endAngle: number; | ||
| fullCircles: number; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| module.exports = { | ||
| config: { | ||
| type: 'doughnut', | ||
| data: { | ||
| labels: [0, 1, 2, 3, 4, 5], | ||
| datasets: [ | ||
| { | ||
| // option in element (fallback) | ||
| data: [5, 2, 4, 7, 6, 8] | ||
| } | ||
| ] | ||
| }, | ||
| options: { | ||
| elements: { | ||
| arc: { | ||
| backgroundColor: 'transparent', | ||
| borderColor: 'black', | ||
| borderWidth: 1, | ||
| borderDash: function(ctx) { | ||
| var value = (ctx.dataIndex || 0) % 2; | ||
| return value === 0 ? [3, 3] : []; | ||
| } | ||
|
|
||
| } | ||
| }, | ||
| } | ||
| }, | ||
| options: { | ||
| canvas: { | ||
| height: 256, | ||
| width: 512 | ||
| } | ||
| } | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| module.exports = { | ||
| config: { | ||
| type: 'polarArea', | ||
| data: { | ||
| labels: [0, 1, 2, 3, 4, 5], | ||
| datasets: [ | ||
| { | ||
| // option in dataset | ||
| data: [5, 2, 4, 7, 6, 8], | ||
| borderAlign: 'inner', | ||
| borderColor: 'black' | ||
| }, | ||
| ] | ||
| }, | ||
| options: { | ||
| elements: { | ||
| arc: { | ||
| backgroundColor: 'transparent', | ||
| borderWidth: 1, | ||
| borderDash: [3, 3] | ||
| } | ||
| }, | ||
| scales: { | ||
| r: { | ||
| display: false | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| options: { | ||
| canvas: { | ||
| height: 256, | ||
| width: 512 | ||
| } | ||
| } | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| module.exports = { | ||
| config: { | ||
| type: 'polarArea', | ||
| data: { | ||
| labels: [0, 1, 2, 3, 4, 5], | ||
| datasets: [ | ||
| { | ||
| // option in dataset | ||
| data: [5, 2, 4, 7, 6, 8] | ||
| } | ||
| ] | ||
| }, | ||
| options: { | ||
| elements: { | ||
| arc: { | ||
| backgroundColor: 'transparent', | ||
| borderColor: 'black', | ||
| borderWidth: 1, | ||
| borderDash: function(ctx) { | ||
| var value = (ctx.dataIndex || 0) % 2; | ||
| return value === 0 ? [3, 3] : []; | ||
| } | ||
| } | ||
| }, | ||
| scales: { | ||
| r: { | ||
| display: false | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| options: { | ||
| canvas: { | ||
| height: 256, | ||
| width: 512 | ||
| } | ||
| } | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| module.exports = { | ||
| config: { | ||
| type: 'polarArea', | ||
| data: { | ||
| labels: [0, 1, 2, 3, 4, 5], | ||
| datasets: [ | ||
| { | ||
| // option in dataset | ||
| data: [5, 2, 4, 7, 6, 8], | ||
| borderAlign: 'inner', | ||
| borderColor: 'black' | ||
| }, | ||
| ] | ||
| }, | ||
| options: { | ||
| elements: { | ||
| arc: { | ||
| backgroundColor: 'transparent', | ||
| borderWidth: 1, | ||
| borderDash: [3, 3] | ||
| } | ||
| }, | ||
| scales: { | ||
| r: { | ||
| display: false | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| options: { | ||
| canvas: { | ||
| height: 256, | ||
| width: 512 | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why
startsWith? and probably does not needhoverBorderDash, as its returned asborderDashwhen hovered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kurkle I used
startsWithto manage alsoborderDashOffsetat the say way ofborderDahbecause the 2 options are strictly related.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.