fix global configuration for hoverBorderWidth#5801
fix global configuration for hoverBorderWidth#5801Niladri24dutta wants to merge 6 commits intochartjs:masterfrom
Conversation
src/controllers/controller.line.js
Outdated
| model.backgroundColor = custom.hoverBackgroundColor || helpers.valueAtIndexOrDefault(dataset.pointHoverBackgroundColor, index, helpers.getHoverColor(model.backgroundColor)); | ||
| model.borderColor = custom.hoverBorderColor || helpers.valueAtIndexOrDefault(dataset.pointHoverBorderColor, index, helpers.getHoverColor(model.borderColor)); | ||
| model.borderWidth = custom.hoverBorderWidth || helpers.valueAtIndexOrDefault(dataset.pointHoverBorderWidth, index, model.borderWidth); | ||
| model.borderWidth = custom.hoverBorderWidth || helpers.valueAtIndexOrDefault(dataset.pointHoverBorderWidth, index, this.chart.options.elements.point.hoverBorderWidth); |
There was a problem hiding this comment.
What if none of custom.hoverBorderWidth, dataset.pointHoverBorderWidth, this.chart.options.elements.point.hoverBorderWidth are set? Don't we still need to fall back to model.borderWidth in that case?
There was a problem hiding this comment.
@benmccann in this case can we use a ternary expression to fall back to model.borderWidth if both the conditions in || are false?
There was a problem hiding this comment.
as per this line of code https://github.com/chartjs/Chart.js/blob/master/src/helpers/helpers.core.js#L70 I was referring to something like this
model.borderWidth = custom.hoverBorderWidth || helpers.valueAtIndexOrDefault(dataset.pointHoverBorderWidth, index, this.chart.options.elements.point.hoverBorderWidth ? this.chart.options.elements.point.hoverBorderWidth : model.borderWidth);
There was a problem hiding this comment.
I think you could write that as:
model.borderWidth = custom.hoverBorderWidth || helpers.valueAtIndexOrDefault(dataset.pointHoverBorderWidth, index, helpers.valueOrDefault(this.chart.options.elements.point.hoverBorderWidth, model.borderWidth));
There was a problem hiding this comment.
Also, since there are two lines that now refer to this.chart.options.elements.point I would refactor that out and create a variable at the beginning of this method with something like: var pointOpts = this.chart.options.elements.point;
There was a problem hiding this comment.
@benmccann I have made the suggested changes in my branch. Can you please let me know if I have missed anything?
|
@benmccann Can you please let me know if my proposed change looks fine, I will make the changes in my branch if it's matches the requirement. |
src/controllers/controller.line.js
Outdated
| var index = element._index; | ||
| var custom = element.custom || {}; | ||
| var model = element._model; | ||
| var pointOpts = this.chart.options.elements.point; |
There was a problem hiding this comment.
there's a trailing space on this line that's causing the build to fail
There was a problem hiding this comment.
this PR looks good to me after the trailing space is fixed
There was a problem hiding this comment.
@benmccann I have removed the extra space and the build is successful . Can you please check now?
|
@benmccann I have made the suggested changes , Can you please check and let me know if anything else required |
|
@benmccann thank you for approving and merging my changes. |
|
@benmccann Hi, I can see that the original issue is still open , is this PR already merged? |
|
@simonbrunel just curious ,will this be merged during the v 2.8 release ? |
simonbrunel
left a comment
simonbrunel
left a comment
There was a problem hiding this comment.
While you are fixing this behavior for borderWidth, can you also fix the surrounding code (I think that's the exact same issue for backgroundColor and borderColor)?
Also, can you update the associated unit tests to prevent regressions?
|
@Niladri24dutta can you rebase this PR and address Simon's comment? |
@benmccann @simonbrunel Yes sure . But I have some doubts regarding the unit test case update . Which unit test cases should I update? I am not able reproduce the same scenario in the unit test cases. |
|
You could probably add it to this one: https://github.com/chartjs/Chart.js/blob/master/test/specs/controller.line.tests.js#L185 If that doesn't work then you could write a new one |
|
Fixed in #5973 |
4 participants
Fixes #5775
hoverBorderWidthwas not applied to chart if it was set at global elements level.