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

Bug: Avoid updating Chart when responsive: true and Chart is hidden. #5172

Merged
merged 2 commits into from
May 23, 2018

Conversation

jcopperfield
Copy link
Contributor

As proposed in PR #4968 a fix to avoid resizing errors when the Chart is hidden.

@benmccann
Copy link
Contributor

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

@@ -192,6 +192,10 @@ module.exports = function(Chart) {

helpers.retinaScale(me, options.devicePixelRatio);

if (newWidth === 0 || newHeight === 0) {
Copy link
Contributor

@benmccann benmccann Jan 25, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@benmccann benmccann Jan 26, 2018

Choose a reason for hiding this comment

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

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

@jcopperfield
Copy link
Contributor Author

jcopperfield commented Jan 25, 2018

@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 me.width === newWidth.

I think it would be OK to combine with the silent check if that is preferred.

@simonbrunel
Copy link
Member

simonbrunel commented Jan 28, 2018

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 display: none or when the canvas size is actually null.

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 draw method should test at L559 if chart width or height is 0 and abort rendering if it's the case.

@benmccann
Copy link
Contributor

@jcopperfield do you plan to update this PR in response to Simon's latest comment?

@benmccann
Copy link
Contributor

@simonbrunel this one is ready for another look now

@benmccann
Copy link
Contributor

@simonbrunel just a reminder this one is waiting on your review. thanks

@simonbrunel simonbrunel requested a review from etimberg May 22, 2018 20:43
@etimberg etimberg added this to the Version 2.8 milestone May 23, 2018
@etimberg etimberg merged commit 25b7f41 into chartjs:master May 23, 2018
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
chartjs#5172)

* Bug: Avoid updating Chart when `responsive: true` and Chart is hidden.

* Prevent `drawing` when width/height is invalid.
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