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

fix/4397 #4406

Merged
merged 4 commits into from Jun 23, 2017
Merged

fix/4397 #4406

merged 4 commits into from Jun 23, 2017

Conversation

Peter-Van-Drunen
Copy link
Contributor

Elements were resizing incorrectly if they were resized while collapsed. Added a check to avoid this issue.

Elements were resizing incorrectly if they were regenerated while collapsed. Added a check to avoid this issue.
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.

Left two minor comments. It'd also be nice to have a test for this that tries to create a chart inside of a collapsed div so that any errors here are prevented in the future

var newHeight = 0;

// Avoid issues with the canvas having negative maximum width and/or height due to element being collapsed
if (Math.floor(helpers.getMaximumWidth(canvas)) >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I would change just just slightly to cache the return of helpers.getMaximumWidth(canvas) into a local variable. This would improve minification and performance. The max width function reads some styles off the canvas element which can be slow

Copy link
Member

Choose a reason for hiding this comment

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

Agree, and could be simply:

var newWidth = Math.max(0, Math.floor(helpers.getMaximumWidth(canvas)));
...

if (Math.floor(helpers.getMaximumWidth(canvas)) >= 0) {
newWidth = Math.floor(helpers.getMaximumWidth(canvas));
}
if (helpers.getMaximumHeight(canvas) >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here about the return of helpers.getMaximumHeight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants