-
Notifications
You must be signed in to change notification settings - Fork 25
feat: subtitles on demand #898
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
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. |
tsi
left a comment
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.
Great work! Approving following our zoom call.
Please make sure you go over the points raised in our call - chapters, playlist-by-tag example (to test source switching), create a small test instead of the removed unit test.
Also, since this new plugin is lazy-loaded, make sure you update index.all.js
| setTimeout(() => { | ||
| textTrack.mode = currentMode; | ||
| }, 0); |
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 have the timeout here?
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, its small improvement for UX - sometimes I noticed that once language is changed its kept until some click so this one triggers repainting
| onSuccess: () => updateTextTrackStatusToSuccess(track), | ||
| onError: (error) => { | ||
| updateTextTrackStatusToError(track, error); | ||
| addNotificationCue(player.duration(), track, 'We could not load selected text track, sorry'); |
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.
Let's confirm this and other UI strings with @njb90
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.
confirmed & replaced
…pt-on-demand # Conflicts: # src/index.all.js # src/video-player.js
I introduced new plugin as replacement for SRT/Paced transcript. The main reason is to add same behaviour for different file types of lazy loading all transcript files.
Key points: