Bug: Avoid updating Chart when responsive: true and Chart is hidden.
#5172
Conversation
|
I'm not super familiar with this section of the code, so maybe an uninformed comment... It seems strange to me that this allows setting the canvas height and width to 0 but then just skips notifying plugins of the change. If this is right, it'd probably help to explain more in a comment why that's the behavior |
src/core/core.controller.js
Outdated
| @@ -192,6 +192,10 @@ module.exports = function(Chart) { | |||
|
|
|||
| helpers.retinaScale(me, options.devicePixelRatio); | |||
|
|
|||
| if (newWidth === 0 || newHeight === 0) { | |||
There was a problem hiding this comment.
you could combine this if statement with the previous one. e.g. make this check if (silent || newWidth === 0 || newHeight === 0) { and then remove the next if statement
There was a problem hiding this comment.
Since you're touching code in this area maybe you can also fix the typo collased a few lines above? I think that comment might also explain why some tests check for width/height of 0
|
@benmccann For some reasons unknown to me there are tests which expect the canvas to have a width/height of 0, otherwise I would have combined it with the check I think it would be OK to combine with the |
|
It seems sane to keep the chart internal sizes in sync with the actual DOM geometry which return 0 for width and height in some cases such as the canvas or containing element(s) have Though, I agree that plugins should be notified of the size change, even if 0. #4968 is about an exception in the drawing code, so I think the |
|
@jcopperfield do you plan to update this PR in response to Simon's latest comment? |
|
@simonbrunel this one is ready for another look now |
|
@simonbrunel just a reminder this one is waiting on your review. thanks |
chartjs#5172) * Bug: Avoid updating Chart when `responsive: true` and Chart is hidden. * Prevent `drawing` when width/height is invalid.
As proposed in PR #4968 a fix to avoid resizing errors when the Chart is hidden.