-
Notifications
You must be signed in to change notification settings - Fork 556
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
update videoJS to 5.4 #11821
update videoJS to 5.4 #11821
Conversation
@@ -36,7 +36,7 @@ | |||
"lodash-amd": "bower_components/lodash-amd/compat/**", | |||
"domready": "bower_components/domready/ready.js", | |||
"when": "bower_components/when/es6-shim/Promise.js", | |||
"video.js": "bower_components/video.js/dist/video.min.js", | |||
"video.js": "bower_components/video.js/dist/video.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure if I have to update this elsewhere for isProd
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, that is all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how come we changed this? Makes it easier to debug, that's for sure—and it's still minised later by Uglify through us :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies! And Olly makes a good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will also have to update the require config:
frontend/grunt-configs/requirejs.js
Line 26 in 9e11025
videojs: 'components/video.js/video.min', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be horrible coming from a jspm world? :-P
CC @Calanthe |
@@ -32,4 +32,4 @@ object EncodingOrdering extends Ordering[Encoding] { | |||
|
|||
//Returns a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second. | |||
def compare(x: Encoding, y: Encoding): Int = precedenceOf(x) - precedenceOf(y) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IDE is doing it due to the editor config.
Hope that was easier than the last upgrade, which included a major version. if all the plugins behave fine, then 👍 |
this reminds me, we need to talk about tests. Enough of this video wild west. (we, as in the wider dotcom team) |
A few of the plugins are actually broken at the moment (our own fullscreener for example). |
Damn it. |
Saying that, I need to chat to @ScottPainterGNM about quality in general. |
I approve of this conversation 😈 |
Broken, broken they are! |
Updating video JS so we can start looking at updating the contribs (mostyle for ads as there are some performance stuff with Google IMA).
I would have like to have update to 5.5, but there is a problem with the autoplay logic here - I have raised an issue and will try to get that out as soon as.
Most contribs require 5.3 - so this is good for now.