fix colour settings of BeforeLabel and BeforeBody #4783
Conversation
| @@ -701,10 +705,8 @@ module.exports = function(Chart) { | |||
| // Inner square | |||
| ctx.fillStyle = mergeOpacity(vm.labelColors[i].backgroundColor, opacity); | |||
There was a problem hiding this comment.
you wouldn't need this line since you end up overriding it later
There was a problem hiding this comment.
Thank you for reviewing.
I revert it and fixed up. 馃崳
Rittyan
force-pushed
the
fix_beforeBody_beforeLabel_colour
branch
from
5edf9b1
to
b159325
Compare
src/core/core.tooltip.js
Outdated
| helpers.each(vm.beforeBody, fillLineOfText); | ||
|
|
||
| var drawColorBoxes = vm.displayColors; | ||
| xLinePadding = drawColorBoxes ? (bodyFontSize + 2) : 0; | ||
|
|
||
| // Draw body lines now | ||
| helpers.each(body, function(bodyItem, i) { | ||
| var labelTextColor = mergeOpacity(vm.labelTextColors[i], opacity); |
There was a problem hiding this comment.
there's no need to have an intermediate variable here. the code below would work and result in a smaller file size
ctx.fillStyle = mergeOpacity(vm.labelTextColors[i], opacity);
There was a problem hiding this comment.
Oops, sorry. I fixed it.
There was a problem hiding this comment.
@etimberg it duplicates your #4766 but I think this one also takes in account my comment on your PR. What do you think?
src/core/core.tooltip.js
Outdated
| helpers.each(vm.beforeBody, fillLineOfText); | ||
|
|
||
| var drawColorBoxes = vm.displayColors; | ||
| xLinePadding = drawColorBoxes ? (bodyFontSize + 2) : 0; | ||
|
|
||
| // Draw body lines now | ||
| helpers.each(body, function(bodyItem, i) { | ||
| ctx.fillStyle = mergeOpacity(vm.labelTextColors[i], opacity); |
There was a problem hiding this comment.
Could be:
var textColor = mergeOpacity(vm.labelTextColors[i], opacity);
ctx.fillStyle = textColor;then we can remove line 706 and avoid calling this mergeOpacity more than one time per body item, right?
There was a problem hiding this comment.
@Rittyan what do you think about the suggested change?
There was a problem hiding this comment.
@simonbrunel
I think it is very good.
May I commit that change?(Include removing line 706)
There was a problem hiding this comment.
@Rittyan yup, you can push that change to this branch
There was a problem hiding this comment.
I fixed it. thanks for reviewing 馃檹
There was a problem hiding this comment.
@simonbrunel I agree with your comment here. I'm happy to merge this once that comment is addressed over my change
* fix colour settings of BeforeLabel and BeforeBody * delete redundant variable declaration * collect label colour setting.
* fix colour settings of BeforeLabel and BeforeBody * delete redundant variable declaration * collect label colour setting.
Hello.
I have found font colour problem when rendering beforeLabel and beforeBody.
Problem
The beforeLabel and beforeBody don't use own colour settings(bodyFontColur,labelTextColor). They use titleFontColor.
jsfiddle:
https://jsfiddle.net/Rittyan/rxhpdyqt/
How fix
It seems missing colour setting before rendering.
I add it.
Sorry I'm not native speaker. so my english broken.
Thanks. 馃檹