-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@MorrisJobke can you test safari |
return $.getScript(scriptPath, function (xhr) {eval(xhr);}); | ||
var scriptPath = OC.filePath('files_videoplayer', 'js', 'videojs/video.js'); | ||
|
||
var deferred = $.Deferred(); |
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.
this is duplicating https://github.com/nextcloud/server/blob/b7767a51f140d3f588278f053f3a9e1eb84e4346/core/js/js.js#L364-L385, isn't it? Does $.getScript
load the script differently?
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.
Yes. I have no idea why or how but it is different. With $.getScript
I did not have a videojs
funtion after loading. This way i do.
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.
@ChristophWurst but it probably would make sense to look into a generic approach here indeed. This is just the quick fix for 15.
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 see, they are indeed a bit different. Let's use this fix for 15 now and look into a proper npm setup for 16 where we can use dependabot and video.js from https://www.npmjs.com/package/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.
We could integrate that with the slideshow/big picture we discussed in nextcloud/server#12382
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.
Yeah, sounds good. So let's npm'ize this repo for 16, switch to Vue and use a standardized component for the overlay.
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.
Code looks good! Haven't tested though.
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.
Works again. 👍
Tested on IE, Edge, Safari, FF, Chromium
Thanks a lot for fixing this @rullzer! 🎉 |
No description provided.