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.

Sign up for GitHub

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

Jump to bottom

Make i18n work with node/browserify #98

Merged
merged 2 commits into from
Aug 19, 2015
Merged

Make i18n work with node/browserify #98

merged 2 commits into from
Aug 19, 2015

Conversation

simon04
Copy link

@simon04 simon04 commented Aug 16, 2015

The locale/language is taken from optional_conf_parm. No direct
interaction with i18next module is required.

@simon04
Copy link
Author

simon04 commented Aug 18, 2015

Have you already found time to consider this pull request?

ypid
ypid reviewed Aug 18, 2015
View reviewed changes
opening_hours.js
@@ -4251,7 +4261,7 @@
user_conf[key] = default_prettify_conf[key];
}

if (typeof user_conf['locale'] === 'string' && user_conf['locale'] !== 'en') {
if (typeof moment !== 'undefined' && typeof user_conf['locale'] === 'string' && user_conf['locale'] !== 'en') {
Copy link
Member

Choose a reason for hiding this comment

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

Please be more specific. E.g. use typeof moment === 'object'.

@simon04
Copy link
Author

simon04 commented Aug 18, 2015

Changed to typeof moment === 'object' …

@ypid
Copy link
Member

ypid commented Aug 18, 2015

Looks good. I just need to make the evaluation tool and the map work again.

You updated the test output. The interesting thing is that when I checkout 72e5f09 I get the old version. I guess this is platform/version depended. What version of NodeJS are you using and what Distro?

ypid
ypid reviewed Aug 18, 2015
View reviewed changes
locales/core.js
@@ -1,3 +1,9 @@
var i18n = i18n;
Copy link
Member

Choose a reason for hiding this comment

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

I don’t quite get that. Is this needed for Nashorn/Java?
For the browser and NodeJS it works without it.

@ypid
Copy link
Member

ypid commented Aug 18, 2015

Well done. Thanks very much …

@ypid ypid added the type: feature Introduction of new functionality. label Aug 18, 2015
@ypid ypid added this to the v3.4.0 milestone Aug 18, 2015
@simon04
Copy link
Author

simon04 commented Aug 19, 2015 

$ iojs --version
v3.0.0
$ npm --version
2.13.3

… using Arch Linux and https://aur.archlinux.org/packages/iojs-bin/

But I get the same output with

$ node --version
v0.12.7
$ npm --version
2.13.5

using NodeJs from the official repository https://www.archlinux.org/packages/community/x86_64/nodejs/

npm list outputs https://gist.github.com/simon04/3f5e21d2beea342aa11a

@simon04
Make i18n work with node/browserify
a52bab0 
The locale/language is taken from `optional_conf_parm`. No direct
interaction with `i18next` module is required.
@simon04
Copy link
Author

simon04 commented Aug 19, 2015

Thank you for your review. I simplified it to var i18n = require('i18next-client');

@ypid ypid merged commit a52bab0 into opening-hours:master Aug 19, 2015
ypid referenced this pull request Aug 19, 2015
@ypid
Reverted test.*.log from before 72e5f09.
805dfab 
@simon04 is using a more recent version of NodeJS then my. Needs to be
fixed later. Test crashes currently anyway so reverting for now.

I am using:
node --version
v0.10.29

(latest in Debian jessie and testing)

https://github.com/ypid/opening_hours.js/pull/98#issuecomment-132466586
ypid added a commit that referenced this pull request Aug 19, 2015
@ypid
Merge branch 'simon04-browserify'. Merged #98. Thanks very much to @s…
3f59829 
…imon04.
@ypid
Copy link
Member

ypid commented Aug 19, 2015

Thanks again and I am looking forward to see the error/warning translation feature in JOSM 👍 😄

ypid referenced this pull request in opening-hours/opening_hours_map Aug 19, 2015
@ypid
Updated to work with https://github.com/ypid/opening_hours.js/pull/98.
f104d71
ypid added a commit that referenced this pull request Sep 6, 2015
@ypid
Updated documentation. Related to #98.
762ce0b
@ypid ypid mentioned this pull request Sep 25, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
type: feature Introduction of new functionality.
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@simon04 @ypid