Skip to content
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

Size options can be overriden #48

Closed
wants to merge 1 commit into from

Conversation

rdolgushin
Copy link

I added the ability to overwrite default size options (for example, if you need to use localized size suffixes).

@rdolgushin rdolgushin closed this Feb 22, 2014
@avoidwork
Copy link
Owner

Is there functionality you'd like to add?

@rdolgushin
Copy link
Author

Yes, I want to add ability to overwrite size options because there is no i18n, but I realized that for this purpose is necessary to overwrite much more parts of filesize and there are many controversial issues, which I can not resolve.
As a result, I decided to make fork with supporting of only one language - Russian (https://github.com/rdolgushin/filesize.js).
If in the future you could to find some way to implement localization support, it would be very good!

@avoidwork
Copy link
Owner

They're based on the international system of units (SI), specifically Metric & JEDEC. Changing the output to a localized language is an interesting request, but I don't know if there's much value in adding it to the core lib. It could be a simple test to perform a string.replace() at the return. Realistically, you don't want to change the existing code.

@rdolgushin
Copy link
Author

Great solution! Little bit slowly, but much more simpler than create ten forks for ten languages. Thanks. I think next time I will do so.

@avoidwork
Copy link
Owner

I have a functional implementation at #49 ... does that look ideal?

@avoidwork
Copy link
Owner

I'm not sure if the key should be 'language', 'locale', or 'l18n'. What do you think?

@rdolgushin
Copy link
Author

I'm not sure if the key should be 'language', 'locale', or 'l18n'. What do you think?

I saw your code and I think that most right name for the key is 'suffixes'.

@rdolgushin
Copy link
Author

I have a functional implementation at #49 ... does that look ideal?

Looks pretty good! 👍

@avoidwork
Copy link
Owner

So far I've pinged 3 coworkers, and we've all got different opinions on what the key should be. Do you know of prior work that handles this kind of overriding? I feel that 'locale' is wrong based on the fact that it's not going to use the system locale setting.

@rdolgushin
Copy link
Author

In many projects that I've seen, such things was named as 'messages' or 'locales'. For example such mainstream framework as Rails (and many Rails-inspired frameworks) has 'locales' - https://github.com/gitlabhq/gitlabhq/blob/master/config/locales/en.yml

@rdolgushin
Copy link
Author

@avoidwork
Copy link
Owner

My original commit had 'lang' as the key, but I changed it to be 'language' for clearer meaning. I'm tempted to leave it as is, unless a compelling argument is made. Please, ask your peers what they think; this is not going to be a commonly used feature, but I would like it to make sense to those that use it.

@rdolgushin
Copy link
Author

My peer (just one) very inspired by Rails and preferred 'locale' (but it does not result in any arguments, moreover, that it is just like him). But I see you want something more accurate. To my mind more accurate and flexible concept will be the 'suffixes', because it is not limited to the scope of the localization - you can add arbitrary suffixes without binding to some language or (may be) locale.

@avoidwork
Copy link
Owner

Ok, unless someone else makes a good argument, I'll change it 'suffixes' in approx 12hr, and this will become 2.0.1.

@rdolgushin
Copy link
Author

Ok. Thank you for your interest about this problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants