Closed putermancer closed 10 years ago
Looks like I need to modify this a bit, the original change was based on an earlier version of ng-i18next and doesn't actually work in its current form. I'll update the PR soon.
I just made a PR for default value, maybe you would like to work above that…
@efolio This pull request (#43) adds the same behavior as your pull request (#47), but also adds support for providing global defaults on the object used when configuring i18nextProvider: $i18nextProvider.options = { ... }
I updated my pull request yesterday and everything is working great on it. I would propose we merge #43 instead of the smaller subset encompassed by #47.
What is defaultLoadingValue
?
I cannot find this global option in ng-i18next or in i18next…
defaultLoadingValue
is a new option added in this pull request. Its purpose is to provide a default value for any key, but only while loading before t(...)
is available.
The point of this option is to prevent untranslated values (the key names) from being displayed while everything is still being loaded. Once everything is ready, this no longer has any effect.
The end result is that no untranslated strings are displayed while loading. But, if you use a key that isn't defined in the translation file, you will still see it show up with just its key name after everything is ready -- in case some people use this to help identify strings that have not been added to the translation file.
On my project, we don't use default values, instead we want to make sure everything is in the translation file. So we don't provide a defaultValue
and we just see the key name in the UI if something is wrong. This allows us to retain that functionality, but not see the key names briefly while things are still loading.
@bloveridge I really like your PR. Having the possibility to have both, a default value for missing keys and a default value for all keys before i18next is ready, is a great idea. :+1:
Regards, Andre
Is it me or the globalOptions.defaultValue
is not supported by i18next?
Otherwise, 'defaultValue' in globalOptions ? globalOptions.defaultValue
does not do what you would expect…
i18next documentation says that defaultValue
is supported but I have not tested yet (I never needed a default value...). //Edit: Well, not in configuration...
But looking at the PR code again, I even wonder whether to check for a defaultValue
and a defaultLoadingValue
makes sense when i18next isn't loaded yet (see these lines). Shouldn't we only look for defaultLoadingValue
and i18next looks for defaultValue
? Because looking defaultValue
when i18next isn't loaded yet makes defaultLoadingValue
useless...
You're right, i18next doesn't support a defaultValue
on the init config object, only per-translation.
So one of a few things should happen:
globalOptions.defaultValue
entirely -- it doesn't do anything except before i18next is initialized, which is misleading.globalOptions.defaultValue
to the subsequent block when passing options to t(...)
for translation.Personally, I like option 2 and the idea of a global default. But it could be argued that this is changing the behavior of i18next more than necessary. Personally I don't have problems with wrapping libraries extending functionality of the underlying library, but others might have a different opinion.
Option 3: make a PR on i18next in order to support defaultValue
on init :)
I'm for Option 3.
Option 4: Option 3 + call the default value (when i18next is loaded, yet) defaultLoadingValue
to avoid misunderstanding, so defaultValue
can be handled by i18next. Both should be available on global configuration and on key configuration.
@archer96 I don't think it's inappropriate to check both defaultValue
and defaultLoadingValue
before i18next is loaded. defaultValue
is on the options
object, so it would be a per-translation default, whereas defaultLoadingValue
is checked on the globalOptions
object.
Effectively, the combination says: "if i18next isn't loaded, still allow the user the specify a defaultValue for a specific translation as normal. But also let them specify a global default until i18next is ready."
@efolio You're right about option 3. I would support this, but in the mean time we would still have to do something inside this repo to make it work until i18next supports it.
@archer96 I'm not quite sure what you mean about Option 4. I think the two are fundamentally different. defaultLoadingValue
would only be used if translations haven't been loaded, whereas adding defaultValue
to init would act like a global default whether the translation file has been loaded or not.
@bloveridge , could you please make another PR:
globalOptions.defaultValue
line (for the time being)loadingDefaultValue
in the READMEloadingDefaultValue
… and if you want, make a PR on i18next's repo?
Ok, I think my English has it's limits ^^
What I mean is following:
Global options contain:
defaultValue
if the translation isn't found (i18next should handle this)defaultLoadingValue
which is used if i18next isn't loaded, yet.Key options should also contain these two values. One if i18next isn't loaded and one if the key isn't found.
@archer96 That's about what I was thinking. Supporting defaultValue
by i18next requires a PR to i18next, or we could add the functionality inside ng-i18next with a small change to the logic before it calls t(...)
.
If we wanted to support that option quickly, we would have to add it in ng-i18next, because we have no control over the PR / release cycle of i18next core.
defaultLoadingValue
is already supported in ng-i18next, and it sounds like you agree it would not make sense to add it to i18next core as the whole purpose of it is to work until i18next is loaded.
I'm working on tests for defaultLoadingValue
right now, and have also removed globalOptions.defaultValue
pending a decision on whether we want to add that logic directly or wait until i18next supports it (if they accept future PR).
Let me summarize:
globalOptions.defaultValue
is not yet supported by i18next, so to be removed for now in my opinionglobalOptions.defaultLoadingValue
yesoptions.defaultValue
yesoptions.defaultLoadingValue
yes, still has to be addedi18next releases before the end of the month and still accepts PRs. I just had one merged a few hours ago.
@efolio That's exactly what I meant :+1:
Not sure I agree on the 4th point, I don't see any good reason to add options.defaultLoadingValue
. options.defaultValue
takes precedence -- I suppose there is a possiblity someone would want their string to say "Loading..." And then be replaced with "Welcome!" -- is that the scenario you have in mind? It doesn't seem too likely to me.
That's what I had in mind.
As the options exist and as they are named, that's the behavior you would expect, so why not let the user decide what he wants to do?
You should add an issue for globalOptions.defaultValue
in order for us to remember that it should be handled after i18next is patched too. :)
@bloveridge Yes. I'm also not sure whether this is usefull or not... But I think it should be available on options
because it's also available on globalOptions
;)
Well, I'm Ok if you both say that it's not usefull ;)
As the options exist and as they are named, that's the behavior you would expect
After the new PR I am making, the only options that exist are the ones already documented in i18next (defaultValue
which is clear is a per-translation option) and defaultLoadingValue
on the initialization object, which will be documented to say it is for happening only before i18next is loaded.
So I'm not sure I follow where things exist / are named that isn't clear or as expected. global defaultValue
isn't documented anywhere -- but I'm happy to make the change to support defaultLoadingValue
inside ng-i18next both for global options and per-translation options.
New PR opened with changes discussed. #49
I've made the necessary changes and PRs for this repo, as I see it.
I will not be making a PR for i18next
for defaultValue
support. While I would be in favor of such a change (as stated earlier), like @archer96 I do not use this functionality and have no personal motivation to do the work right now, nor justification to do it on my employer's time as I have done this work :)
I have tried various combinations of
ng-bind
,ng-bind-attr
,ng-cloak
,ng-i18next
, etc. to avoid displaying key names very briefly while i18next is still loading, but could never get to the point where the value wasn't actually injected into the DOM until the actual translation was available. This pull request introduces two minor changes that helped me solve the problem (or related problems).defaultValue
beforet(...)
is availablet(...)
defaultLoadingValue
that can be used beforet(...)
is available.