Add 'forceLocalizationDefaults' config option #3128
Conversation
test/unit/language-test.js
Outdated
| @@ -337,6 +341,30 @@ describe('languageUtils', function() { | |||
| expect(customLocalization.pause).to.equal(localizationPause); | |||
| expect(customLocalization.stop).to.equal(localizationStop); | |||
| }); | |||
|
|
|||
| it('should only use custom localization block if "forceLocalizationDefaults" is true', function() { | |||
| stubHtmlLanguage(document, 'fr'); | |||
There was a problem hiding this comment.
No need to stub the language on the hmtl, we can stub getLanguage to return 'fr'
import the entire language module import * as Language from 'utils/language';
sandbox.stub(Language, 'getLanguage').returns('fr');
…rce-english-localization
test/unit/language-test.js
Outdated
| intl | ||
| }); | ||
| const configLocalization = config.localization; | ||
| expect(Language.getLanguage()).to.equal('fr'); |
There was a problem hiding this comment.
we don't need to test this; it would mean we are testing the stub
| @@ -119,7 +119,7 @@ const Config = function(options, persisted) { | |||
| _copyToLocalization(allOptions); | |||
| _deserialize(allOptions); | |||
|
|
|||
| const language = getLanguage(); | |||
| const language = allOptions.forceLocalizationDefaults ? Defaults.language : getLanguage(); | |||
There was a problem hiding this comment.
What is the value of Defaults.language? Why don't we just set a language in the config, and base the test on setupConfig.language? I much rather we use existing config options and support the setting of language in the config, than add a new option that no one need like forceLocalizationDefaults.
There was a problem hiding this comment.
Defaults.language is 'en'. I have mixed feelings about providing a config option to override language because we want to support google translate, which overrides the language tag on the page. If the player is setup after google translate then we read the updated language tag.
if the language is being overriden via the config by the developer, then we have to count on them to update it when the page has been translated, which i doubt they would do.
test/unit/language-test.js
Outdated
| @@ -353,18 +343,23 @@ describe('languageUtils', function() { | |||
| }); | |||
|
|
|||
| it('should only use custom localization block if "forceLocalizationDefaults" is true', function() { | |||
| sandbox.stub(Language, 'getLanguage').returns('fr'); | |||
There was a problem hiding this comment.
@DanFerrer why did you remove the stubbing ? If we don't stub then how are we verifying that we override the language when forceLocalizationDefaults is true ?
There was a problem hiding this comment.
I think you might have misunderstood our conversation from friday. What i meant to say was that expect(Language.getLanguage()).to.equal('fr'); is unnecessary since get language is being stubbed and we don't need to unit test the stubbing.
In order to properly test that forceLocalizationDefaults overrides the language, we need to stub getLanguage. Does this make sense ?
There was a problem hiding this comment.
@DanFerrer I am approving but let's wait for @robwalch's response in https://github.com/jwplayer/jwplayer/pull/3128/files#r225222580 before merging
This PR will...
forceLocalizationDefaultstrueintlblock if given.Why is this Pull Request needed?
New a/b test
Are there any points in the code the reviewer needs to double check?
Are there any Pull Requests open in other repos which need to be merged with this?
https://github.com/jwplayer/borg/pull/1056
https://github.com/jwplayer/jwplayer-commercial/pull/5859
Addresses Issue(s):
JW8-2260