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.

Sign up for GitHub

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

Jump to bottom

Update to fix responsive chart size in IE11 when parent container has… #4620

Merged
merged 13 commits into from Apr 22, 2018
Merged

Update to fix responsive chart size in IE11 when parent container has… #4620

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ca9b1ae
Update to fix responsive chart size in IE11 when parent container has…
andi-b Aug 7, 2017
3d4fe3a
Updated code formating
andi-b Aug 8, 2017
5528a61
Updated code formatting
andi-b Aug 8, 2017
26a56d7
Updated spaces to tabs
andi-b Aug 8, 2017
f31af0e
Added calculatePadding function
andi-b Jan 16, 2018
52cdcce
Reduced code further
andi-b Jan 16, 2018
083175b
Updated to inline if statement
andi-b Jan 16, 2018
4bfe92d
Testing script added for calculatePadding function
andi-b Feb 7, 2018
23b863c
Merge branch 'master' into patch-1
andi-b Feb 7, 2018
cd00af3
Update code formatting
andi-b Feb 7, 2018
fedda5b
Added variables and made function private
andi-b Feb 9, 2018
ec79652
Removed calls to private function
andi-b Feb 9, 2018
76d0c0c
Removed unused variable
andi-b Feb 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/core/core.helpers.js
View file
Open in desktop
Expand Up @@ -466,14 +466,20 @@ module.exports = function(Chart) {
helpers.getConstraintHeight = function(domNode) {
return getConstraintDimension(domNode, 'max-height', 'clientHeight');
};
helpers.calculatePadding = function(container, padding, parentDimension) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't want this method to be part of the public API.

Can you prefix the method with _ and add a private comment:

/**
 * @private
 */
helpers._calculatePadding = function(...

padding = helpers.getStyle(container, padding);

return padding.indexOf('%') > -1 ? parentDimension / parseInt(padding, 10) : parseInt(padding, 10);
};
helpers.getMaximumWidth = function(domNode) {
var container = domNode.parentNode;
if (!container) {
return domNode.clientWidth;
}

var paddingLeft = parseInt(helpers.getStyle(container, 'padding-left'), 10);
var paddingRight = parseInt(helpers.getStyle(container, 'padding-right'), 10);
var paddingLeft = helpers.calculatePadding(container, 'padding-left', container.clientWidth);
Copy link
Member

Choose a reason for hiding this comment

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

We should cache clientWidth since it's used multiple times:

var clientWidth = container.clientWidth;
var paddingLeft = helpers.calculatePadding(container, 'padding-left', clientWidth );
var paddingRight = helpers.calculatePadding(container, 'padding-right', clientWidth );
var w = clientWidth  - paddingLeft - paddingRight;
// ...

var paddingRight = helpers.calculatePadding(container, 'padding-right', container.clientWidth);

var w = container.clientWidth - paddingLeft - paddingRight;
var cw = helpers.getConstraintWidth(domNode);
return isNaN(cw) ? w : Math.min(w, cw);
Expand All @@ -484,8 +490,9 @@ module.exports = function(Chart) {
return domNode.clientHeight;
}

var paddingTop = parseInt(helpers.getStyle(container, 'padding-top'), 10);
var paddingBottom = parseInt(helpers.getStyle(container, 'padding-bottom'), 10);
var paddingTop = helpers.calculatePadding(container, 'padding-top', container.clientHeight);
Copy link
Member

Choose a reason for hiding this comment

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

Same here (var clientHeight = container.clientHeight;)

var paddingBottom = helpers.calculatePadding(container, 'padding-bottom', container.clientHeight);

var h = container.clientHeight - paddingTop - paddingBottom;
var ch = helpers.getConstraintHeight(domNode);
return isNaN(ch) ? h : Math.min(h, ch);
Expand Down
35 changes: 35 additions & 0 deletions test/specs/core.helpers.tests.js
View file
Open in desktop
Expand Up @@ -742,6 +742,41 @@ describe('Core helper tests', function() {
expect(canvas.style.width).toBe('400px');
});

it ('Should get padding of parent as number (pixels) when defined as percent (returns incorrectly in IE11)', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we would need a unit test that doesn't check internals but creates a chart inside a container with percent padding and verify the chart dimensions (something similar to this test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed call to now private function calculatePadding.
I attempted to write test with chart but require a parent DIV to the wrapper so I can set the width as the padding percentage is taken from this and not from the wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it('**********Should calculate padding of parent container in pixels if as percent (IE11)', function(done) {
	var chart = acquireChart({
		type: 'pie',
		options: {
			responsive: true,
			maintainAspectRatio: true
		}
	}, {
		canvas: {
			style: 'width: 300px; height: 300px; position: relative'
		},
		wrapper: {
			style: 'padding: 10%; width: 300px; height: 300px;'
		}
	});

	expect(chart).toBeChartOfSize({
		dw: 240, dh: 240,
		rw: 240, rh: 240,
	});
});


// Create div with fixed size as a test bed
var div = document.createElement('div');
div.style.width = '300px';
div.style.height = '300px';
document.body.appendChild(div);

// Inner DIV to have 10% padding of parent
var innerDiv = document.createElement('div');

div.appendChild(innerDiv);

var canvas = document.createElement('canvas');
innerDiv.appendChild(canvas);

var container = canvas.parentNode;

// No padding
expect(helpers.calculatePadding(container, 'padding-right', container.clientWidth)).toBe(0);
expect(helpers.getMaximumWidth(canvas)).toBe(300);

// test with percentage
innerDiv.style.padding = '10%';
expect(helpers.calculatePadding(container, 'padding-right', container.clientWidth)).toBe(30);
expect(helpers.getMaximumWidth(canvas)).toBe(240);

// test with pixels
innerDiv.style.padding = '10px';
expect(helpers.calculatePadding(container, 'padding-right', container.clientWidth)).toBe(10);
expect(helpers.getMaximumWidth(canvas)).toBe(280);

document.body.removeChild(div);
});

describe('Color helper', function() {
function isColorInstance(obj) {
return typeof obj === 'object' && obj.hasOwnProperty('values') && obj.values.hasOwnProperty('rgb');
Expand Down