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

Add player.currentWidth and player.currentHeight* methods for getting computed style #3144

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,67 @@ class Component {
return parseInt(this.el_['offset' + toTitleCase(widthOrHeight)], 10);
}

/**
* Get width or height of computed style
* @param {String} widthOrHeight 'width' or 'height'
* @return {Number|Boolean} The bolean false if nothing was set
* @method currentDimension
*/
currentDimension(widthOrHeight) {
let computedWidthOrHeight;
Copy link
Member

Choose a reason for hiding this comment

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

also, the default for this variable should probably be 0.

Copy link
Author

Choose a reason for hiding this comment

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

ok.


if (widthOrHeight !== 'width' && widthOrHeight !== 'height') {
log.warn('currentDimension only accepts width or height value');
return false;
Copy link
Member

Choose a reason for hiding this comment

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

It probably should return null or NaN instead of a boolean.
NaN would make a more consistent return signature, null is easier to check for.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we probably should throw an error in this case.

Copy link
Author

Choose a reason for hiding this comment

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

I have thrown an exception.

}

if (typeof window.getComputedStyle !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

safer to check that it's equal to "function".

Copy link
Author

Choose a reason for hiding this comment

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

ok.

const computedStyle = window.getComputedStyle(this.el_, null);
Copy link
Member

Choose a reason for hiding this comment

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

the null isn't necessary.

Copy link
Author

Choose a reason for hiding this comment

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

ok.

computedWidthOrHeight = computedStyle.getPropertyValue(widthOrHeight) || computedStyle[ widthOrHeight ];
Copy link
Member

Choose a reason for hiding this comment

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

Very minor style comment: spaces inside [ and ].

Copy link
Member

Choose a reason for hiding this comment

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

This can be fixed during merge; so, ignore me!

}

if (this.el_.currentStyle) {
Copy link
Member

Choose a reason for hiding this comment

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

should be an else if from the getComputedStyle.

Copy link
Author

Choose a reason for hiding this comment

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

ok.

// ie 8 doesn't support computed style, shim it
// return clientWidth or clientHeight instead for better accuracy
const rule = 'client' + widthOrHeight.substr(0, 1).toUpperCase() + widthOrHeight.substr(1);
Copy link
Member

Choose a reason for hiding this comment

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

would offsetWidth/offsetHeight be better?
Also, we have a utility for this called toTitleCase (and it's already imported.

const rule = `client${toTitleCase(widthOrHeight)}`

Copy link
Author

Choose a reason for hiding this comment

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

I have converted to offsetWidth.

computedWidthOrHeight = this.el_[rule];
}

// remove 'px' from variable and parse as integer
computedWidthOrHeight = parseInt(computedWidthOrHeight, 10);
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 use parseFloat instead since we could get fractional values.

Copy link
Author

Choose a reason for hiding this comment

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

ok.

return computedWidthOrHeight;
}

/**
* Get an object which contains width and height values of computed style
* @return {Object} The dimensions of element
* @method currentDimensions
*/
currentDimensions() {
return {
width: this.currentDimension('width'),
height: this.currentDimension('height')
};
}

/**
* Get width of computed style
* @return {Integer}
* @method currentWidth
*/
currentWidth() {
return this.currentDimension('width');
}

/**
* Get height of computed style
* @return {Integer}
* @method currentHeight
*/
currentHeight() {
return this.currentDimension('height');
}

/**
* Emit 'tap' events when touch events are supported
* This is used to support toggling the controls through a tap on the video.
Expand Down
28 changes: 28 additions & 0 deletions test/unit/component.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,34 @@ test('should change the width and height of a component', function(){
ok(comp.height() === 0, 'forced height was removed');
});

test('should get the computed dimensions', function(){
var container = document.createElement('div');
Copy link
Member

Choose a reason for hiding this comment

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

we try to use let and const in the newer test code we write.

Copy link
Author

Choose a reason for hiding this comment

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

ok.

var comp = new Component(getFakePlayer(), {});
var el = comp.el();
var fixture = document.getElementById('qunit-fixture');

fixture.appendChild(container);
container.appendChild(el);
// Container of el needs dimensions or the component won't have dimensions
container.style.width = '1000px';
container.style.height = '1000px';

comp.width('50%');
comp.height('50%');

var computedWidth = TestHelpers.getComputedStyle(el, 'width');
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 probably hard code these values since otherwise we're basically testing that getComputedStyle returns what getComputedStyle returs which is a bit more tautological for my tastes :)

Copy link
Author

Choose a reason for hiding this comment

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

ok 👍

var computedHeight = TestHelpers.getComputedStyle(el, 'height');

ok(computedWidth === comp.currentWidth() + 'px', 'matches computed width');
Copy link
Member

Choose a reason for hiding this comment

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

it would be better to use the equal directive from qunit.

equal(comp.currentWidth() + 'px', computedWidth, 'matches computed width');

Copy link
Author

Choose a reason for hiding this comment

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

ok.

ok(computedHeight === comp.currentHeight() + 'px', 'matches computed height');

ok(computedWidth === comp.currentDimension('width') + 'px', 'matches computed width');
ok(computedHeight === comp.currentDimension('height') + 'px', 'matches computed height');

ok(computedWidth === comp.currentDimensions()['width'] + 'px', 'matches computed width');
ok(computedHeight === comp.currentDimensions()['height'] + 'px', 'matches computed width');

});

test('should use a defined content el for appending children', function(){
class CompWithContent extends Component {}
Expand Down