Filter sources type and sort by resolution #10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you very much. I love when I'm about to custom write something and I find a perfect pull request already exists! Much appreciated! |
|
Great PR! |
|
are there any updates on that? |
|
Any updates? |
izkmdz
suggested changes
Feb 23, 2022
| @@ -54,18 +54,35 @@ module.exports = function(videojs) { | |||
| this.update(); | |||
| }, | |||
|
|
|||
| orderSources: function(a, b) { | |||
There was a problem hiding this comment.
- Instead of using a public sort method for
orderSources, perhaps it would be more appropriate to make it a locally-scoped anonymous function like the one in.filteron line 74. - For readability, renaming the
resattribute toresolutionwill help clarify its purpose. - If one or more sources do not have the
resolutionattribute, we should keep the sources in the same order.
| singleTypeSources, | ||
| orderedSources; | ||
|
|
||
| /* filter sources by the current type on the player */ |
There was a problem hiding this comment.
Minor suggestion, we prefer to start comments with // instead of using /* */. Thanks!
| return item.type === player.currentType(); | ||
| }); | ||
|
|
||
| /* sort sources by res attributed if exist */ |
There was a problem hiding this comment.
There is a typo, by res attributed if exist can be changed to by resolution attribute if it exists
izkmdz
suggested changes
Feb 23, 2022
| @@ -54,18 +54,35 @@ module.exports = function(videojs) { | |||
| this.update(); | |||
| }, | |||
|
|
|||
| orderSources: function(a, b) { | |||
| if (a.res && b.res) { | |||
| return parseFloat(a.res) - parseFloat(b.res); | |||
There was a problem hiding this comment.
By default, can the sort order be descending with the largest resolution at the top? Thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This small addition allows you to use this plugin with multiple types of videos ( for example mp4 and webm ) and also order the labels by passing additional argument of "res" for resolution (that is optional ).
Filtering by type:
Functionality checks the current type on the player and then filters the sources by the type so you not ending up with duplicated labels for each source type.
Sorting by resolution:
The part for sorting labels checks if the argument of "res" exist and if it is it orders the labels by values of that attribute for example [{ lebel: High, res: 1400 }, { lebel: Low, res: 360 }, { lebel: Medium, res: 750 } ], so menu alway be Low, Medium, High. If there is no "res" it diplays the order as it comes.
All those changes are very helpful for the plugin to be used with multiple types and in conjunction with plugins like Language Switch where you replace sources depending on language selection and then quality labels order is all random.