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

Default dimensions #2482

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
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
5 changes: 0 additions & 5 deletions src/css/components/_layout.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@
vertical-align: top;
box-sizing: border-box;

/* Default to the video element width/height. This will be overridden by
* the source width height unless changed elsewhere. */
width: 300px;
height: 150px;

color: $primary-text;
background-color: $primary-bg;
position: relative;
Expand Down
26 changes: 14 additions & 12 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import log from './utils/log.js';
import toTitleCase from './utils/to-title-case.js';
import { createTimeRange } from './utils/time-ranges.js';
import { bufferedPercent } from './utils/buffer.js';
import * as stylesheet from './utils/stylesheet.js';
import FullscreenApi from './fullscreen-api.js';
import MediaError from './media-error.js';
import safeParseTuple from 'safe-json-parse/tuple';
Expand Down Expand Up @@ -274,8 +275,10 @@ class Player extends Component {
// Add a style element in the player that we'll use to set the width/height
// of the player in a way that's still overrideable by CSS, just like the
// video element
this.styleEl_ = document.createElement('style');
el.appendChild(this.styleEl_);
this.styleEl_ = stylesheet.getStyleElement('vjs-styles-dimensions');
let defaultsStyleEl = document.querySelector('.vjs-styles-defaults');
let head = document.querySelector('head');
head.insertBefore(this.styleEl_, defaultsStyleEl ? defaultsStyleEl.nextSibling : head.firstChild);
Copy link
Member

Choose a reason for hiding this comment

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

Should we worry about scenarios where defaultsStyleEl is the only thing in the head. Or if there is no head for some reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's worth worrying about if there's no head. I think looking for the defaultsStyleEl and prepending to HEAD otherwise, is quick and easy enough to do.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.


// Pass in the width/height/aspectRatio options which will update the style el
this.width(this.options_.width);
Expand Down Expand Up @@ -448,17 +451,16 @@ class Player extends Component {
// Ensure the right class is still on the player for the style element
this.addClass(idClass);

// Create the width/height CSS
var css = `.${idClass} { width: ${width}px; height: ${height}px; }`;
// Add the aspect ratio CSS for when using a fluid layout
css += `.${idClass}.vjs-fluid { padding-top: ${ratioMultiplier * 100}%; }`;
stylesheet.setTextContent(this.styleEl_, `
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using one template string here to add both the previous css selectors to the style element.

Copy link
Member

Choose a reason for hiding this comment

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

Beautiful.

.${idClass} {
width: ${width}px;
height: ${height}px;
}

// Update the style el
if (this.styleEl_.styleSheet){
this.styleEl_.styleSheet.cssText = css;
} else {
this.styleEl_.innerHTML = css;
}
.${idClass}.vjs-fluid {
padding-top: ${ratioMultiplier * 100}%;
}
`);
}

/**
Expand Down
16 changes: 16 additions & 0 deletions src/js/utils/stylesheet.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import document from 'global/document';

export let getStyleElement = function(className) {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be named createStyleElement, otherwise it looks like you're getting an existing style el with the supplied class name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. I guess it could possibly look to see if a style element with the same class name exists and return that and create one, otherwise, or something, but I don't think that's that useful currently.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, since each player needs its own style element I don't think we need a getter. Which reminds me, should we have something that cleans up the style el when the player is disposed? Previously I think it was just relying on the style el being removed with the player div.

Copy link
Member Author

Choose a reason for hiding this comment

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

probably. I can add that in along with this rename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

let style = document.createElement('style');
style.className = className;

return style;
};

export let setTextContent = function(el, content) {
if (el.styleSheet) {
el.styleSheet.cssText = content;
} else {
el.textContent = content;
}
};
11 changes: 11 additions & 0 deletions src/js/video.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import document from 'global/document';
import * as setup from './setup';
import * as stylesheet from './utils/stylesheet.js';
import Component from './component';
import EventTarget from './event-target';
import Player from './player';
Expand Down Expand Up @@ -94,6 +95,16 @@ var videojs = function(id, options, ready){
return tag['player'] || new Player(tag, options, ready);
};

// Add default styles
let style = stylesheet.getStyleElement('vjs-styles-defaults');
let head = document.querySelector('head');
head.insertBefore(style, head.firstChild);
stylesheet.setTextContent(style, `
.video-js {
width: 300px;
height: 150px;
`);

// Run Auto-load players
// You have to wait at least once in case this script is loaded after your video in the DOM (weird behavior only with minified version)
setup.autoSetupTimeout(1, videojs);
Expand Down
1 change: 0 additions & 1 deletion test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ test('should set the width, height, and aspect ratio via a css class', function(
};

// Initial state
ok(player.styleEl_.parentNode === player.el(), 'player has a style element');
ok(!getStyleText(player.styleEl_), 'style element should be empty when the player is given no dimensions');
Copy link
Member Author

Choose a reason for hiding this comment

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

we're no longer adding this element to the player. The tests below should cover whether the styles are applied correctly or not.


// Set only the width
Expand Down