Default dimensions #2482

gkatsev · 2015-08-14T16:59:51Z

Remove default dimensions from the stylesheet. Add them back in when videojs loads.
Add move the dimensions stylsheet into the head of the document.

pam · 2015-08-14T17:05:40Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: c705b59
Build details: https://travis-ci.org/pam/video.js/builds/75635241

(Please note that this is a fully automated comment.)

gkatsev · 2015-08-14T19:40:18Z

heff [13:34]
@gkatsev: the stylesheet changes are pretty cool, and seem powerful, but also feel like overkill for this use case. What are the benefits of switching from just overwriting cssText.

gkatsev [14:11]
@heff: I think that it we should be using the DOM APIs that are given to us instead of using cssText/innerHTML for just anything. The extra code allows us to simplify the usage of adding the css in js and making it use cssText instead of the API won't really save us anything.

heff [14:21]
It would save us a pretty sizable chunk of code. I think the DOM API argument would matter to me if we were modifying single rules, but we’re still clearing out the entire sheet and rewriting it, just now we’re doing it in two steps instead of one, with a lot of extra code. Doesn’t seem the most optimal.

gkatsev [14:22]
I think

[
      ['.' + idClass,
        ['width', width + 'px'],
        ['height', height + 'px']
      ],
      ['.' + idClass + '.vjs-fluid',
        ['padding-top', (ratioMultiplier*100) + '%']
      ]
    ])

gkatsev [14:23]
I think that is much cleaner than

gkatsev [14:23]

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}%; }`;

gkatsev [14:23]
and most of the extra code goes into allowing that

heff [14:24]
Maybe we should get another opinion on that because I can read the second more easily.

gkatsev [14:37]
also, in SVT, we use an addCssRule type function for adding our fullscreen/fluid-iframe styles

gkatsev [14:43]
@here thoughts on my PR and discussion above? (#2482)

pat [14:45]
in this case, i find the second more readable. though, i do have some sympathy for the desire to use the DOM APIs rather than more blunt tools

pat [14:46]
hard to say. no strong opinion on this from me :simple_smile:

mmcc [15:14]
the first example looks an attempt to write lisp with square brackets

gkatsev [15:14]
more like css

mmcc [15:15]
eh?

mmcc [15:15]
I don't know if I'm missing something, but I think the second looks more like css

mmcc [15:15]
just with interpolation happening

mmcc [15:18]
@gkatsev: Like @pat, I agree with you on why it's a good idea, but the second option is more readable

gkatsev [15:18]
T__T

mmcc [15:18]
lol I'm not saying that's the end of the conversation by any means

gkatsev [15:18]
I can get rid of that and still be using insertRule

gkatsev [15:19]
stylesheet.addCssRule(selector, properties);

gkatsev [15:19]

properties = `width: ${width}px; height: ${height}px;`

mmcc [15:21]
that seems clean to me. Any reason you don't like that?

gkatsev [15:22]

the array method was available on MDN. 2) I like the array method because it will generate the properties string for you, you don't need to think about it. Just list the names and values

mmcc [15:24]
got it

gkatsev [15:24]
also, I'm a fan of lisp, so... 😛

mmcc [15:24]
I don't have really strong feelings either way, but even knowing exactly what's going in that array syntax I still struggle to follow

gkatsev [15:25]
idk, the array syntax looks pretty much like css but with extra brackets in there

mmcc [15:25]
squints really hard

pat [15:26]
that's a fair argument - that you're writing js, not css, so why should it look like css
gkatsev
[15:26]
Added Untitled in #general

['.video-js',
  ['width', '300px'],
  ['height', '150px']
]

.video-js {
  width: 300px;
  height: 150px;
}

135b JavaScript/JSON • Edit • New window • View raw • Add comment

mmcc [15:26]
@pat: because ultimately you're writing CSS

mmcc [15:27]
the end result is what other devs are having to parse there, which is the CSS output that the code will create

gkatsev [15:27]
don't those two look pretty similar?

mmcc [15:28]
@gkatsev: similar in the same way that ruby looks similar to javascript

mmcc [15:28]
by that I mean, I can look at those to things and functionally know that they do similar things

mmcc [15:28]
if I know to look that they do similar things

mmcc [15:29]
if that makes sense

pat [15:29]
the js style is harder to break because its structure is enforced vs. an interpolated string which is prone to eval-ish problems (bad syntax, etc)

gkatsev · 2015-08-14T19:58:57Z

So, proposal:
remove code that supported the nested array structure for generating the css and instead just have it accept strings.
So, instead of:

    stylesheet.addCssRules(this.styleEl_.sheet, [
      ['.' + idClass,
        ['width', width + 'px'],
        ['height', height + 'px']
      ],
      ['.' + idClass + '.vjs-fluid',
        ['padding-top', (ratioMultiplier*100) + '%']
      ]
    ]);

You would do:

stylesheet.addCssRule(this.styleEl.sheet, '.' + idClass, `width: ${width}px; height: ${height}px;`);
stylesheet.addCssRule(this.styleEl.sheet, `.${idClass}.vjs-fluid`, `padding-top: ${ratioMultiplier * 100}%`);

It would still use insertRule behind the scenes. Basically, only thing that will be left is the addRule method in stylesheet.js.

heff · 2015-08-14T21:51:05Z

Can you test that this direction works in IE8? styleEl.sheet doesn't exist, but I think you can use styleEl.styleSheet. Would be good to confirm the rest of it works.

If this lightens the added code I'll concede the direction. Still feels like we're using a spoon instead of a shovel to dig the same hole. It'd be nice if we could point to some other difference besides one DOM API is more right than the other, because that still feels debatable.

gkatsev · 2015-08-14T22:10:40Z

I'll take a look at the .sheet thing in IE8. Though, we already use it elsewhere and I don't think we've had issues.

I'll think about it some more but generally I much prefer using an actual API, if available, over just setting the inner text of an element.

pam · 2015-08-17T15:58:19Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 8d7d48612bda09bef32f86724aa3717ff06afc34
Build details: https://travis-ci.org/pam/video.js/builds/75959270

(Please note that this is a fully automated comment.)

pam · 2015-08-17T15:59:09Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 26e55c3
Build details: https://travis-ci.org/pam/video.js/builds/75959348

(Please note that this is a fully automated comment.)

gkatsev · 2015-08-17T17:08:24Z

test/unit/player.test.js

@@ -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');


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

gkatsev · 2015-08-17T17:09:28Z

This one is good to go, I think.

pam · 2015-08-17T17:13:27Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 01191ed
Build details: https://travis-ci.org/pam/video.js/builds/75970038

(Please note that this is a fully automated comment.)

gkatsev · 2015-08-17T17:38:30Z

The test failure is a timeout.

heff · 2015-08-17T19:28:33Z

src/js/player.js

+    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);


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

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.

Sounds good.

heff · 2015-08-17T19:52:24Z

lgtm!

pam · 2015-08-17T19:53:05Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: c020041
Build details: https://travis-ci.org/pam/video.js/builds/75992694

(Please note that this is a fully automated comment.)

gkatsev · 2015-08-17T19:54:29Z

@pam retry

pam · 2015-08-17T19:59:45Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 58088ba
Build details: https://travis-ci.org/pam/video.js/builds/75993403

(Please note that this is a fully automated comment.)

…of HEAD el. closes videojs#2482

gkatsev added 2 commits August 13, 2015 16:52

remove default dimensions from css

229a60a

Add better stylesheet handling

c705b59

Switch back to style.textContent

b81bba2

gkatsev force-pushed the default-dimensions branch from 8d7d486 to 26e55c3 Compare August 17, 2015 15:55

The player no longer has this style element

26e55c3

Use a set text content helper for stylsheets

01191ed

gkatsev reviewed Aug 17, 2015
View reviewed changes

heff reviewed Aug 17, 2015
View reviewed changes

gkatsev added 3 commits August 17, 2015 15:44

rename getStyleElement to createStyleElement

cc2511a

remove the player's personal stylesheet

c020041

Check to styleEl existence

58088ba

gkatsev added a commit to gkatsev/video.js that referenced this pull request Aug 19, 2015

@gkatsev moved default and player dimensions to style els at the top …

ecca3a1

…of HEAD el. closes videojs#2482

gkatsev closed this in 76b5ffc Aug 19, 2015

gkatsev deleted the default-dimensions branch August 19, 2015 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default dimensions #2482

Default dimensions #2482

gkatsev commented Aug 14, 2015

pam commented Aug 14, 2015

gkatsev commented Aug 14, 2015

gkatsev commented Aug 14, 2015

heff commented Aug 14, 2015

gkatsev commented Aug 14, 2015

pam commented Aug 17, 2015

pam commented Aug 17, 2015

gkatsev Aug 17, 2015

gkatsev commented Aug 17, 2015

pam commented Aug 17, 2015

gkatsev commented Aug 17, 2015

heff Aug 17, 2015

gkatsev Aug 17, 2015

heff Aug 17, 2015

heff commented Aug 17, 2015

pam commented Aug 17, 2015

gkatsev commented Aug 17, 2015

pam commented Aug 17, 2015

Default dimensions #2482

Default dimensions #2482

Conversation

gkatsev commented Aug 14, 2015

pam commented Aug 14, 2015

gkatsev commented Aug 14, 2015

gkatsev commented Aug 14, 2015

heff commented Aug 14, 2015

gkatsev commented Aug 14, 2015

pam commented Aug 17, 2015

pam commented Aug 17, 2015

gkatsev Aug 17, 2015

Choose a reason for hiding this comment

gkatsev commented Aug 17, 2015

pam commented Aug 17, 2015

gkatsev commented Aug 17, 2015

heff Aug 17, 2015

Choose a reason for hiding this comment

gkatsev Aug 17, 2015

Choose a reason for hiding this comment

heff Aug 17, 2015

Choose a reason for hiding this comment

heff commented Aug 17, 2015

pam commented Aug 17, 2015

gkatsev commented Aug 17, 2015

pam commented Aug 17, 2015