feat: source switcher #904
Conversation
✅ Deploy Preview for cld-vp-esm-pages ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for cld-video-player ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| const empty = new MenuItem(this.player_, { | ||
| label: this._emptyLabel || 'No sources', | ||
| selectable: false | ||
| }); | ||
| empty.addClass('vjs-source-switcher-empty'); | ||
| empty.disable(); | ||
| return [empty]; |
There was a problem hiding this comment.
why do we have an "empty" state?
There was a problem hiding this comment.
initially this component is mounted without any source, just to avoid complex implementation and just after that it gets list of sources provided as player params (so its the part of player, it should never be visible but just in case empty state is added rather than nothing)
|
@jakub-roch how will this play together with a |
@tsi for now we dont really care about any incorrect usage (like blocking other methods when source is being used), user can provide publicId but it will be ignored and first item of I confirmed with Raz that we will handle all the issues reported by users once they occur as for now it will too many stuff that we would to need to mape to prevent "incorrect" usage |
@jakub-roch why using the Also, please add this to internal analytics |
@tsi We need to think about that as its the same question of - whats the point of using source() when playlist is provided? Thanks for reminder about internal analytics! |
tsi
left a comment
tsi
left a comment
There was a problem hiding this comment.
Looks and works great! 💪
https://codepen.io/tsi/pen/qEOyQGr
| }); | ||
| } | ||
|
|
||
| setItems(items, keepSelection = true) { |
There was a problem hiding this comment.
Why keepSelection defaults to false if its only usage needs false? looks like this can be simplified
There was a problem hiding this comment.
good point 👍 I fixed it
| <li><a href="./seek-thumbs.html">Seek Thumbnails</a></li> | ||
| <li><a href="./share-plugin.html">Share & Download</a></li> | ||
| <li><a href="./shoppable.html">Shoppable Videos</a></li> | ||
| <li><a href="./source-switcher.html">Source switcher</a></li> |
There was a problem hiding this comment.
Should we also add an ESM example page? I'm ok with not having everything doubled, just that this is what we did with other examples
There was a problem hiding this comment.
we dont have yet E2E test for this page (just mocked for now) but I believe ESM should be added only if there is a reason for that (otherwise we are doubling everything), for now I will keep just one page but we need to decide about some rules here (will schedule meeting for us after gathering)
src/validators/validators.js
Outdated
| }, | ||
| playerOptions: { | ||
| debug: validator.isBoolean, | ||
| videoSources: validator.isArray, |
| withCredentials: playerOptions.withCredentials, | ||
| debug: playerOptions.debug, | ||
| type: playerOptions.type, | ||
| videoSources: !!playerOptions.videoSources, |
Source switcher