openpsa / jsgrid

Fork of last jqGrid version before license change
http://openpsa.github.io/jsgrid/
Other
28 stars 12 forks source link

i18n english locale #69

Closed bouks closed 9 years ago

bouks commented 9 years ago

Should we include default english in grid.base.js as it was done in lasts commits ?

If so, shouldn't we remove the english locale in i18n ?

Duplicates are not cool.

In the actual state, some users would include english locale and therefore add unnecessary request to server.

meh-uk commented 9 years ago

I didn't include the default English in the codebase as I thought there was a consensus against it.

flack commented 9 years ago

Hm, maybe I merged it with some of @OlegKi's without noticing it, but it's there in current HEAD:

https://github.com/openpsa/grid.js/blob/master/js/grid.base.js#L20

bouks commented 9 years ago

@flack Yes it's there, so that's why my question. :)

meh-uk commented 9 years ago

IMO I think we should remove it.

flack commented 9 years ago

The file or the inlined version?

smartcorestudio commented 9 years ago

That's the reason for that I think, it was in this Oleg's commit: https://github.com/openpsa/grid.js/commit/c02ac04c14c1e17747dd635a285d344495f6b368

bouks commented 9 years ago

Two possibilities :

The first avoid english spoken people to include one more file.

meh-uk commented 9 years ago

The second makes it easy for people who aren't supporting English.

bouks commented 9 years ago

Not much easy for them, this will be the same as now. No ?

flack commented 9 years ago

I can see why it might be nice for new users to have one less thing to worry about, but it makes the jqgrid file bigger than it has to be for everyone not using English.

Just as a sidenote: Removing the en file would cause 404s with my server code:

$lang = "en";
$language = midcom::get()->i18n->get_current_language();
if (file_exists(MIDCOM_STATIC_ROOT . $jqgrid_path . 'js/i18n/grid.locale-' . $language . '.js'))
{
    $lang = $language;
}
$head->add_jsfile(MIDCOM_STATIC_URL . $jqgrid_path . 'js/i18n/grid.locale-'. $lang . '.js');

It basically tries to load the file for the current language, and if it's not available, it falls back to en. I'm not sure how many people have a similar loading mechanism, but I thought I'd mention it.

bouks commented 9 years ago

@flack

For what you raise :

Note that the file get a little bigger, but, in case of english, it avoids an include and then one more server call. :)

Like we say in french : "on ne peut pas avoir le beurre et l'argent du beurre" that in google translation make (i hope it's good translation): "have your cake and butter money"

:)

flack commented 9 years ago

I think the English equivalent would be "You can't have your cake and eat it, too". Google Translate est nul des fois :-)

About the PHP code: Sure, it is a trivial fix. I just wanted to let you know that there might be some side effects on the server side. But we can add that to the release notes if we remove the file

meh-uk commented 9 years ago

We are also asking for yet another serverside change for existing users if we include it. And they should be avoided.

meh-uk commented 9 years ago

Plus I'd have thought special casing English to not include the locale was worse than including it.

On the other hand now we've corrected the language codes that code will need changing anyway so .

bouks commented 9 years ago

I think we just have to vote.

I vote for nothing : ) . My only concern is not to have duplicates and confusion.

flack commented 9 years ago

the funny thing is that at least in my case, removing the en locale file makes my server code shorter:

$language = midcom::get()->i18n->get_current_language();
if (file_exists(MIDCOM_STATIC_ROOT . $jqgrid_path . 'js/i18n/grid.locale-' . $language . '.js'))
{
    $head->add_jsfile(MIDCOM_STATIC_URL . $jqgrid_path . 'js/i18n/grid.locale-'. $language . '.js');
}

For reasons of symmetry, it would be nice to keep the en file. But if it only includes redundant information, then we should remove it. In short: I am undecided, too :-) Maybe we should flip a coin at the call later today. That is still planned, isn't it?

flack commented 9 years ago

P.S.: Another option would be to solve this in the build system: We could keep the en strings in the separate file, and then just add it as the first file when the JS is concatenated. This would also mean that in the download builder, we could offer the option to select the default grid locale, i.e. if someone wants French, they would get the fr file included in grid.min.js, thus having the benefit of one less request, while still avoiding the unneeded weight of an unused locale file

bouks commented 9 years ago

@flack yes would be cool. In charge of the backend dev to include dynamically other file like you did.

flack commented 9 years ago

The only problem I see is that the bundled locale would probably override the other locale file, e.g. if you have something like this:

<script src="grid-locale-ru.js"></script>
<script src="grid-with-bundled-en-locale.min.js"></script>

Then the bundled en would probably overload the ru file. So we would need to add some kind of detection for that situation (but that shouldn't be too hard to do)

flack commented 9 years ago

This is implemented now as discussed. The behaviour is as follows:

Documentation still needs to be updated, but download builder is online

flack commented 9 years ago

P.S.: and if you have a custom locale in your build, and you include a separate locale file before grid.min.js, the built-in default gets overridden by the file

meh-uk commented 9 years ago

Sounds good :)