-
Notifications
You must be signed in to change notification settings - Fork 154
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
fix(Plural): A logic bug in plural #313
base: master
Are you sure you want to change the base?
Conversation
@abruzzihraig I'm pretty sure that functionality works. Nonetheless, if we want to get this in, you'll need to add a unit test, which fails without the fix applied. |
So if you pretty sure that functionality works, why I can get this: I was totally followed by the developer guide. On Wed, Sep 21, 2016 at 18:25 Ruben Vermeersch [email protected]
|
Note that we have a test that tests exactly this: angular-gettext/test/unit/directive.js Lines 134 to 143 in 7d7ac67
|
@rubenv when input bind to something like i think you can have a look at the page below :) also cc @sebrojas14 |
@@ -313,7 +313,7 @@ angular.module('gettext').factory('gettextCatalog', ["gettextPlurals", "gettextF | |||
var fallbackLanguage = gettextFallbackLanguage(this.currentLanguage); | |||
string = this.getStringFormFor(this.currentLanguage, string, n, context) || | |||
this.getStringFormFor(fallbackLanguage, string, n, context) || | |||
prefixDebug(n === 1 ? string : stringPlural); | |||
prefixDebug(n == 1 ? string : stringPlural); |
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.
but i think it would be better if convert n
to number :/
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.
also, there should be a new test case for this.
update: living demo on jsfiddle goes here: https://jsfiddle.net/hxv260y6/ |
Guys, I found a plural bug when I was developing with your library.
In your original code there is a line:
That means the 'n' must be a Number rather than a String or something else.
But how did you get the 'n'? I found that you did something like this:
That means if the translateN on attrs is a String, the 'n' will be String as well. Then you will never get a right plural result when the count changing.
So I just made the change to ensure the logic is safe.
And I think maybe it also fixed the issue #305 and the similar other ones.