search

angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.81k stars 27.5k forks source link

Bug/feature-request when passing decimal-numbers in strings to number-filters #15137

Open filipbech opened 8 years ago

filipbech commented 8 years ago

When you pass a string with a number with decimals to one of the number filters (currency, number) its unclear what happens...

It uses Math.abs() to convert it to a number, which I guess makes sense, but I wish it would do it based on the locale. If you are, like me, are from a country that uses commas as separators you expect the string "4,95" to work, but with math.abs() thats turns into NaN which means nothing shows up.

Possible solutions:

I made a plunker, but it only shows that the last number doesn't show up.

filipbech commented 8 years ago

I made a quick PR to show the solution I prefer (the second where it just works everywhere the same).

https://github.com/angular/angular.js/compare/master...filipbech:patch-1

Narretz commented 8 years ago

I think that's a reasonable change (unless I'm missing something). You should open a PR with this change + tests for the new behavior.

filipbech commented 8 years ago

@Narretz are you referring to my second bullet - making it work for all strings (commas or dots)?

If so, I will get on to writing some tests, even though thats not my expertise.. but I will give it a shot...

Narretz commented 8 years ago

Yes I meant the 2nd suggestion

gkalpak commented 8 years ago

(Basides the fact that the proposed implementation is not correct (i.e. we need Math.abs() in all cases), I am not sure how I feel about this. The problem is that not all locales use , as their separator.

There are some that use \u066b (which looks like a ,, but won't be replaced). Even if we did a search-replace for all decimal separators that in our currently supported locales, there is no guarantee that it won't change in the future. It feels like treating some locales "better" than others.

On the other hand, I must admit it would be a useful addition (instead of having to manually replace the decimal separators before passing the value to the filter).

gkalpak commented 8 years ago

Maybe it makes most sense to replace the current locales decimal separator with . (so that JS can parse it as a number). If users need to format strings that use a separator that is neither . nor the current locales separator, then they need to replace it themselves.

filipbech commented 8 years ago

I thought it was always either . or ,
Your idea sounds better then. I'll give it a try and comment back