-
Notifications
You must be signed in to change notification settings - Fork 53
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
refactor: Use ES6 classes instead of videojs.extend #93
Conversation
dc1264f
to
68eb394
Compare
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.
@absidue Thank you for the PR. Overall it looks good, we just need to maintain backward compatibility with browsers that do not support ES6+ class syntax. Similar work is being done in https://github.com/silvermine/videojs-airplay/pull/44/files where the author added Babelify to transpile the class syntax to ES5. Could you use that PR as a guide to do the same?
The other changes in that PR, such as the tests, are nice to have but not necessary if you're not inclined to include them.
Can you please also update the icon to \f114 as done in pr #99? With videojs 8 it is showing the incorrect "download file" icon instead of the gears. |
Thanks so much because this truly fixed the issue I had of incompatibility between video.js last version and this plugin which rendered it unusable. |
@gsimko Changing that icon to a video.js 8 specific one, will probably break it for video.js 7 and below. |
@absidue that's right but without adding video-js version-specific code it's not possible to maintain backward compatibility because it's a breaking change in video-js. In such cases, people often decide to cut a new semantic version and leave behind the burden of supporting the old version, but it's up to the owner of this package whether actively supporting old versions is desired. |
Are there any updates on this? Thank you |
Just to clarify, if the icon change needs further discussion, we should get this PR submitted without touching that part. Users can circumvent the icon problem, but not the breaking problem that this PR fixes. |
My suggestion would be to get this PR merged and released, then open a follow-up PR that addresses the breaking change introduced in Video.js 8 with regard to the icon (this could then be a new major release that marks dropping Video.js 7 support as breaking change) Matt is probably busy so maybe @pbredenberg or @onebytegone can decide whether this PR is good to go as is or needs additional work |
Hello, |
Thank you @absidue! |
Hello, @yokuze can you please make an update by npm? |
In which version is this merged? CDNs are serving v1.1.2 :? |
Hello, what's the status on this? On video.js 8.6.0 it's still showing the videojs.extend is not a function problem. |
Hi, Is the problem resolved? |
No. If I install via npm, the problem is not solved. That's why I can't upgrade to the new version of videojs. |
As a workaround until a new release is available on npm: Yarn can fetch packages from a git repository. Include the following line in your project's "videojs-quality-selector": "https://github.com/silvermine/videojs-quality-selector/archive/d16842b1acb16cb25f12404ce56d8577b1c61a8d.tar.gz", Then overwrite the wrong font icon (download icon) to restore the cog icon again (as per #93 (comment)): .vjs-quality-selector {
.vjs-icon-placeholder::before {
content: "\f114";
}
} |
@yokuze When should we expect a new release in npm? |
video.js 8 removes the
videojs.extend
function and recommends using native ES6 classes. Recent versions of video.js already spit out deprecation warnings when that function is used.https://videojs.com/guides/videojs-7-to-8/#videojsextend