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

menu-config internationalization #428

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

biodivAtlasAT
Copy link

@biodivAtlasAT biodivAtlasAT commented Oct 27, 2021

Dear community,

I tried to do internationalization to the menu-config.json
by adding i18nNr variable refering to the message.properties numbers of spatial-hub

I changed menuServcie.js when menu data is loaded to:

if ($SH.menu.indexOf('http') == 0) {
var setup = $http.get($SH.menu, _httpDescription('getMenu')).then(function (data) {
var it_0 = 0;
var it_1 = 0;
for(it_0 = 0; it_0 < data.data.length; it_0++) {
data.data[it_0].name = $i18n(data.data[it_0].i18nNr);
for(it_1 = 0; it_1 < data.data[it_0].items.length; it_1++) {
data.data[it_0].items[it_1].name = $i18n(data.data[it_0].items[it_1].i18nNr);
}
}
menuConfig = data.data;
return menuConfig;
});
} else {
menuConfig = $SH.menu;
setup = $q.when(menuConfig)
}

Excuse me, but there are other unneeded changes in this too - I applied a window.getCookie('lang') for switching language
all over the atlas which won't be needed by all of us

We added the String names of the menu-config.json file into our message.properties within incemented numbers

best regards
Georg Neubauer

Reinhardt and others added 11 commits April 9, 2020 09:17
# Conflicts:
#	grails-app/assets/javascripts/spApp/service/i18nService.js
#	grails-app/i18n/messages_de.properties
_Events.groovy
def p = new File(baseDir + '/grails-app/i18n/messages.properties')
    // test refere to data dir
    //def p = new File('/data/spatial-hub/i18n/messages.properties')
toDo spatial service on linux language to complete!
within using the message.properties numbers
as variables in addition to menu-config.json

"name": "Name"
"i18nNr": "number"
_Events.groovy Outdated
println 'Starting NPM install'
final workdir = new File(baseDir, '')
final proc = new ProcessBuilder().inheritIO()
final exec = proc.command('npm', '-dd', 'install').start()
// npm for UNIX, npm.cmd for Windows
final exec = proc.command('npm.cmd', '-dd', 'install').start()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see several problems in the PR. This is not related with menu-config internationalization. Please send a PR for this npm issue ensuring that your change doesn't affect others.

If we merge this, will only work in Windows (not as before).

So find a solution that works for Windows and Linux/Mac systems (detecting the platform or similar).

With a PR using upstream code with only this change, will be easy to review and merge. The same with others changes.

Copy link
Contributor

@qifeng-bai qifeng-bai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw run.cmd has been fixed. Shall we only cherry-pick internationalisation part or merge all? @vjrj

@qifeng-bai qifeng-bai self-requested a review October 31, 2021 22:15
@@ -25,22 +25,22 @@
}
},
{
"description": "Species options.",
"description": $i18n(411),
Copy link
Contributor

@qifeng-bai qifeng-bai Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a default value, like: $i18n(411, "Species options."). It would give developers a direct hint : it is "Species options.".

I have added a new $i18n(k, v) method to support this functionality

qifeng-bai added a commit that referenced this pull request Nov 1, 2021
@biodivAtlasAT
Copy link
Author

biodivAtlasAT commented Nov 2, 2021 via email

@qifeng-bai
Copy link
Contributor

qifeng-bai commented Nov 3, 2021

o! If the default value is the english descriptive name it will be easier to match the descriptive names of the other languages and we wouldn’t have to match them over the numbers.

Yes, we agree.

At this moment, we use both ways, since historically grails i18n pipeline plugin automatically generates i18n key-value pair by using number as key.

Please check our latest commits (i18n) in our dev branch. What we changed:

Add a new function $i18n = function (k, v) in spApp.js . Add a new parameter v, if cannot find key in i18n properties, use V as default. - also, it can be a hint when the key is a number. for example :
$i18n(411, "Species options.").
if 411 cannot be found, use "Species options."

In menuService.js, we merged your code for dynamic menu with small changes -> using name as key, not your i18nNr

In message.properties, we added few examples of 'descriptive names' as key
Add\ to\ map=Add to map

we also support $i18n("Add to map")

If 'Add to map' cannot be found in message.properties, it will use 'Add to map' as value

Cheers

@biodivAtlasAT
Copy link
Author

biodivAtlasAT commented Nov 3, 2021 via email

qifeng-bai added a commit that referenced this pull request Nov 5, 2021
qifeng-bai added a commit that referenced this pull request Nov 5, 2021
@vjrj
Copy link
Contributor

vjrj commented Jan 19, 2022

I saw run.cmd has been fixed. Shall we only cherry-pick internationalisation part or merge all? @vjrj

Sincerely, I still see a mix of things here, and I see difficult to cherry-pick, but it's my impression. Also other minor things should be addressed like:

image

@qifeng-bai
Copy link
Contributor

Thanks, @vjrj . I will follow up it after I finish my current tasks

@qifeng-bai qifeng-bai mentioned this pull request Feb 18, 2022
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.

3 participants