add check on overwriting canvas height/width #4874
Conversation
|
@andersponders sorry about the test flakiness. If you rebase against the latest master hopefully it should be fixed |
|
@andersponders would you be able to add a unit test for this issue? |
|
yea i can do that |
| @@ -544,8 +544,10 @@ module.exports = function(Chart) { | |||
| // If no style has been set on the canvas, the render size is used as display size, | |||
| // making the chart visually bigger, so let's enforce it to the "correct" values. | |||
| // See https://github.com/chartjs/Chart.js/issues/3575 | |||
| canvas.style.height = height + 'px'; | |||
| canvas.style.width = width + 'px'; | |||
| if (!canvas.style.height && !canvas.style.width) { | |||
There was a problem hiding this comment.
@andersponders shouldn't we split this check in case only one dimension is set in the style?
There was a problem hiding this comment.
You could, but I would say if the user is explicitly setting one through CSS, it seems more natural to let the other get handled that way too. im too lazy to make an example, but in my case I was setting height to be 70% of the container, and in that case i probably don't want the library to set width to be static 200px
There was a problem hiding this comment.
i should add this is with responsive=false, another option is instead check if responsive is set
There was a problem hiding this comment.
This method should not be aware about the responsive stuff, let's keep it the way you did.
|
+1 for unit tests, but also please make sure that doesn't break #3575 |
|
added simple test case |
There was a problem hiding this comment.
@andersponders sorry for the delay, was waiting to release 2.7.1, then forgot about that PR. It looks good to me.
* add check on overwriting canvas height/width * unit test for this
* add check on overwriting canvas height/width * unit test for this
#4872