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

Correct CZ and SK locales (finally) #233

Merged
merged 2 commits into from Feb 13, 2017

Conversation

Projects
None yet
4 participants
@smajl
Copy link

smajl commented Feb 6, 2017

Based on my old (and never merged..) pull request to numeral.js. This update fixes incorrect abbreviations for Czech and Slovak locales. Also uses non-breaking space for thousands delimiter (as properly used in CZ and SK). Future proofed with spaceSeparated currency symbol set to true.

Abbreviations check:
http://www.unicode.org/cldr/charts/28/verify/numbers/cs.html
http://www.unicode.org/cldr/charts/29/verify/numbers/sk.html

Tests updated and passing.

BTW, is there an easy way to use non-breaking space for separating a currency symbol?

@@ -2,7 +2,7 @@
* numbro.js language configuration
* language : Czech
* locale: Czech Republic
* author : Anatoli Papirovski : https://github.com/apapirovski

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 6, 2017

Owner

maybe have 2 authors?
as still a big part of this file comes from @apapirovski , I think it would be fair 😄

This comment has been minimized.

@smajl

smajl Feb 7, 2017

Well, some foreigner (I guess Russian living in Canada) copies a template, changes currency symbol and two abbreviations... well, OK, he put some effort into it, I will change the mention. :D

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 7, 2017

Owner

thanks 😄

I try to consider every contribution, even a small one as valuable input 😄

This comment has been minimized.

@apapirovski

apapirovski Feb 7, 2017

Contributor

@smalj Russian who grew up in the Czech Republic, but not sure why that's pertinent anyway. If you want to remove the credit, I don't really give a crap but there's no need to be a sore douche about it.

I didn't see you contributing a Czech localization 4 years ago. Never claimed it was perfect or thorough, but it was what I needed for my own purposes back then.

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 7, 2017

Owner

come on guys, let's try to calm down.

As I said, every contribution is valuable. And no contribution is perfect.

Let's put both names as I think both deserves it.

This comment has been minimized.

@apapirovski

apapirovski Feb 7, 2017

Contributor

I'm perfectly calm, I'm just waiting to hear from @smalj why my nationality and current location is relevant to any of this. Hadn't had it become a part of a pull request before this.

As for the errors that were there in the localization, numeral-js didn't support the spaceSeparated param. I had to fix the library in the first place to allow putting the currency symbol after the number rather than before. The support for shortened versions of million & billion was added a year or two after I worked on this.

This comment has been minimized.

@smajl

smajl Feb 8, 2017

@apapirovski Sorry, that was not meant to be personal, calm your RU/CZ/CA temper. ;)) I will give you credit back, no problem. Should have used blame/annotate, my fault, the file undergone a lot of changes. I did not know that you also added support for currency symbol suffix, good job (tap tap on shoulder).

decimal: ','
},
abbreviations: {
thousand: 'tis.',
million: 'mil.',
billion: 'b',
trillion: 't'
billion: 'mld.',

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 6, 2017

Owner

do you have so documentation about that? 😄

It's quite hard to check if it makes sense 😄

This comment has been minimized.

@smajl

smajl Feb 7, 2017

Have you seen those two links I posted? ;)

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 7, 2017

Owner

just did, sorry 😄

@@ -11,21 +11,22 @@
langLocaleCode: 'cs-CZ',
cultureCode: 'cs-CZ',
delimiters: {
thousands: ' ',
thousands: '\u00a0',

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 6, 2017

Owner

I am not quite sure about that, as if the output string is escaped (as I think it should always be to avoid surprises, then it will look weird)

Between quotes will be a unicode nbsp " " so maybe copy/pasting that could make it (copied from there)

This comment has been minimized.

@smajl

smajl Feb 7, 2017

OK, will change to unicode nbsp char.

This comment has been minimized.

@smajl

smajl Feb 7, 2017

Mmm, does not work for me with unicode nbsp. On the other hand \u00a0 behaves nicely, producing   in between. Why would you escape the product of the numbro.js lib? Isn't much smarter to escape directly the raw data? Can you please give me an example where would the \u00a0 cause a problem? Thanks.

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Feb 7, 2017

Owner

I consider a good practice to not just use string given back by a library in order to avoid XSS and any other issue 😄

So I tend to always escape string I get from users and/or libraries to avoid this.
And in this case, that would produce not nice looking output 😄

This comment has been minimized.

@BenjaminVanRyseghem

This comment has been minimized.

@NicolasPetton

NicolasPetton Feb 7, 2017

Contributor

@BenjaminVanRyseghem I think it's ok to expect users of numbro to escape user input if they need to, but not numbro strings. If there's a simple way to make it work using the unicode char, then it'd solve all issues at once :)

This comment has been minimized.

@smajl

smajl Feb 13, 2017

@BenjaminVanRyseghem So how do we resolve this? Seems like @NicolasPetton has the same idea that developer should more likely escape user input and not output of numbro (does not make sense to me at all). Unfortunately, I can't make it to work with unicode char to please everyone. So can we merge this or should I remove this change and maybe put it in separate merge request somewhere in the future?

@BenjaminVanRyseghem

This comment has been minimized.

Copy link
Owner

BenjaminVanRyseghem commented Feb 13, 2017

let's do it this way, and we will see if anyone complains 😄

@BenjaminVanRyseghem BenjaminVanRyseghem merged commit 8b8a9d2 into BenjaminVanRyseghem:develop Feb 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@NicolasPetton

This comment has been minimized.

Copy link
Contributor

NicolasPetton commented Feb 13, 2017

@smajl

This comment has been minimized.

Copy link

smajl commented Feb 13, 2017

@NicolasPetton I mean, why would I want to escape output from an open-source library which is specialised in formatting strings and I can check it's source code and expect reasonable outputs from it?

@NicolasPetton

This comment has been minimized.

Copy link
Contributor

NicolasPetton commented Feb 13, 2017

@BenjaminVanRyseghem

This comment has been minimized.

Copy link
Owner

BenjaminVanRyseghem commented Mar 27, 2017

in numbro 1.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment