Add player.currentWidth and player.currentHeight* methods for getting computed style
#3144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add `player.currentWidth` and `player.currentHeight`* methods for getting computed style
## Specific Changes proposed
API methods:
Component
Component.prototype.currentDimension(widthOrHeight) -> Number - It takes a string value of either width or height to return back the appropriate value
Component.prototype.currentDimensions() -> Object{width: Number, height: Number} - Returns an object with width and height properties
Component.prototype.currentHeight() -> Number - Delegates to Component#currentDimension to get back the height value
Component.prototype.currentWidth() -> Number - Delegates to Component#currentDimension to get back the width value
## Requirements Checklist
- [ X ] Feature implemented / Bug fixed
- [ X ] If necessary, more likely in a feature request than a bug fix
- [ X ] Unit Tests updated or fixed
- [ ] Docs/guides updated
- [ ] Example created ([starter template on JSBin](http://jsbin.com/axedog/edit?html,output))
- [ ] Reviewed by Two Core Contributors
|
Thanks. Sorry it's taken so long to take a look. At a quick glance, this looks good. I'll make some more in-depths comments tomorrow. |
|
@gkatsev you're welcome 👍 after your full review, I am going to fix issues. |
| return false; | ||
| } | ||
|
|
||
| if (typeof window.getComputedStyle !== 'undefined') { |
There was a problem hiding this comment.
safer to check that it's equal to "function".
|
I made some comments. Let me know if you have questions or need any help! |
|
@gkatsev I fixed issues. |
|
LGTM, thanks. |
gkatsev
added
minor
|
@videojs/core-committers anyone else care to do a code review? |
|
|
||
| if (typeof window.getComputedStyle === 'function') { | ||
| const computedStyle = window.getComputedStyle(this.el_); | ||
| computedWidthOrHeight = computedStyle.getPropertyValue(widthOrHeight) || computedStyle[ widthOrHeight ]; |
There was a problem hiding this comment.
Very minor style comment: spaces inside [ and ].
There was a problem hiding this comment.
This can be fixed during merge; so, ignore me!
|
LGTM |
gkatsev
added
confirmed
and removed
needs: LGTM
|
@defli thanks for your help! Looking forward to more contributions from you :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add
player.currentWidthandplayer.currentHeight* methods for getting computed styleSpecific Changes proposed
API methods:
Component
Component.prototype.currentDimension(widthOrHeight) -> Number - It takes a string value of either width or height to return back the appropriate value
Component.prototype.currentDimensions() -> Object{width: Number, height: Number} - Returns an object with width and height properties
Component.prototype.currentHeight() -> Number - Delegates to Component#currentDimension to get back the height value
Component.prototype.currentWidth() -> Number - Delegates to Component#currentDimension to get back the width value
Player - We probably don't need to make any changes in the player because it extends from component and will inherit the functionality from it.
Requirements Checklist