Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CanvasPattern support to tooltip #4284

Closed
wants to merge 1 commit into from

Conversation

swierczek
Copy link

@swierczek swierczek commented May 24, 2017

Fix for #4279 and subsequently ashiguruma/patternomaly#10

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should also check for gradients as well. Would be nice to have some tests covering this code and a fiddle that demonstrates what happens after the change. I think this might have some visual artifacts because the opacity is not merged

@@ -691,7 +691,14 @@ module.exports = function(Chart) {
ctx.strokeRect(pt.x, pt.y, bodyFontSize, bodyFontSize);

// Inner square
ctx.fillStyle = mergeOpacity(vm.labelColors[i].backgroundColor, opacity);
var bgColor = vm.labelColors[i].backgroundColor;
if (bgColor instanceof CanvasPattern) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good idea to check for CanvasGradient as well too

@ChrisKatsaras
Copy link

@swierczek @etimberg Any update on merging this?

@etimberg
Copy link
Member

@ChrisKatsaras my comments above need to be addressed before merging and tests need to be written. I'm not sure if @swierczek planned to do them or not.

@ChrisKatsaras
Copy link

I agree! @swierczek any idea when you will implement these changes?

@swierczek
Copy link
Author

Checking it out now, sorry this was part of another project and once I figured it out I made the PR and promptly forgot about it ;)

@benmccann
Copy link
Contributor

@swierczek do you still plan to make those updates?

@swierczek
Copy link
Author

I took a stab at it a while back but again it fell off my radar... someone else is more than welcome to make these additional changes instead of waiting on me

@etimberg
Copy link
Member

Closing due to inactivity

@etimberg etimberg closed this Dec 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants