-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat(FEC-11964): Allow configuring player to auto-select audio track according to browser locale #640
Conversation
…according to browser locale
@@ -691,7 +691,7 @@ var config = { | |||
> | |||
> > ### config.playback.audioLanguage | |||
> > | |||
> > ##### Type: `string` | |||
> > ##### Type: `string || "auto"` |
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.
you mean the default is auto ?
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.
no. string means language like 'en' etc. or 'auto'
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.
not sure I understand, "auto" is a value and not a type
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.
right it's a value, specific value so it's documented explicitly
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.
it's already in the description, why does it need to be in the type as well ?
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.
no special reason, just to be aligned with the text language documentation - https://github.com/kaltura/playkit-js/blob/master/docs/configuration.md#type-string--auto
src/player.js
Outdated
* @private | ||
* @returns {string} - The track language to set by default. | ||
*/ | ||
_getLanguage(configuredLanguage: string, defaultTrack: ?TextTrack): string { | ||
_getLanguage(configuredLanguage: string, defaultTrack: ?(TextTrack | AudioTrack)): string { |
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 think that writing it like in _setDefaultTrack, with a typed function and passing the tracks from outside, is cleaner than having an if inside the function.
And if we don't use language outside _setDefaultTrack anyway, you can also merge _setDefaultTrack with _getLanguage, or call _getLanguage from inside _setDefaultTrack.
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.
done
src/player.js
Outdated
@@ -2548,25 +2548,29 @@ export default class Player extends FakeEventTarget { | |||
const activeTracks = this.getActiveTracks(); | |||
const playbackConfig = this.config.playback; | |||
const offTextTrack: ?Track = this._getTextTracks().find(track => TextTrack.langComparer(OFF, track.language)); | |||
let currentOrConfiguredTextLang = this._playbackAttributesState.textLanguage || this._getLanguage(playbackConfig.textLanguage, activeTracks.text); | |||
let currentOrConfiguredAudioLang = this._playbackAttributesState.audioLanguage || playbackConfig.audioLanguage; | |||
let currentOrConfiguredTextLang = |
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.
const
src/player.js
Outdated
let currentOrConfiguredTextLang = | ||
this._playbackAttributesState.textLanguage || | ||
this._getLanguage<TextTrack>(this._getTextTracks(), playbackConfig.textLanguage, activeTracks.text); | ||
let currentOrConfiguredAudioLang = |
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.
const
Description of the Changes
add the option to set
playback.audioLanguage: 'auto'
and compare with the locale thenSolves FEC-11964
CheckLists