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

Automatically generate i18n javascript files for react-intl when the serve starts up. #642

Merged
merged 1 commit into from
Jan 12, 2017

Conversation

JasonYCHuang
Copy link
Contributor

@JasonYCHuang JasonYCHuang commented Dec 10, 2016

This PR is for shakacode/react-webpack-rails-tutorial#340
We need i18n_dir & default_locale in ReactOnRails configuration.


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 82.792% when pulling 38e92e2 on JasonYCHuang:i18n into 6370447 on shakacode:master.

@justin808
Copy link
Member

we want this pr to include i18n.rake


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@justin808
Copy link
Member

@JasonYCHuang How's this PR going?

@JasonYCHuang
Copy link
Contributor Author

@justin808 I am occupied recently, and will finish this during Xmas or New year holidays.

@JasonYCHuang JasonYCHuang force-pushed the i18n branch 3 times, most recently from fdff007 to c367290 Compare December 27, 2016 10:02
@coveralls
Copy link

coveralls commented Dec 27, 2016

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling c367290 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling c367290 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling c367290 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@justin808
Copy link
Member

@JasonYCHuang Looks potentially super amazing! We need updates to the docs and a CHANGELOG.md, as well as tests that demonstrate and verify locals_to_js.rb.

This is really a huge addition to the project! 👏 🎉


Reviewed 2 of 5 files at r2.
Review status: 2 of 5 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@JasonYCHuang
Copy link
Contributor Author

No problem, will update it later.


Review status: 2 of 5 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@JasonYCHuang JasonYCHuang changed the title add i18n_dir & default_locale to configuration Automatically generate javascript files for react-intl when the serve starts up. Dec 31, 2016
@JasonYCHuang JasonYCHuang changed the title Automatically generate javascript files for react-intl when the serve starts up. Automatically generate i18n javascript files for react-intl when the serve starts up. Dec 31, 2016
@JasonYCHuang JasonYCHuang force-pushed the i18n branch 2 times, most recently from d2188f1 to 95d8a08 Compare December 31, 2016 13:08
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 95d8a08 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

1 similar comment
@coveralls
Copy link

coveralls commented Dec 31, 2016

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 95d8a08 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@coveralls
Copy link

coveralls commented Dec 31, 2016

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 95d8a08 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@JasonYCHuang JasonYCHuang force-pushed the i18n branch 4 times, most recently from ffacf2c to 2870647 Compare December 31, 2016 13:36
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 2870647 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 2870647 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@coveralls
Copy link

coveralls commented Dec 31, 2016

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 2870647 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@JasonYCHuang
Copy link
Contributor Author

Update Readme, changelog, and add test


Review status: 2 of 8 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Dec 31, 2016

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 2870647 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@coveralls
Copy link

coveralls commented Dec 31, 2016

Coverage Status

Coverage increased (+0.02%) to 99.316% when pulling 2870647 on JasonYCHuang:i18n into 13cff3f on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.321% when pulling 37d1a2a on JasonYCHuang:i18n into 8db37b1 on shakacode:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.321% when pulling 37d1a2a on JasonYCHuang:i18n into 8db37b1 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.32% when pulling 093a996 on JasonYCHuang:i18n into 8db37b1 on shakacode:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.32% when pulling 093a996 on JasonYCHuang:i18n into 8db37b1 on shakacode:master.

@coveralls
Copy link

coveralls commented Jan 8, 2017

Coverage Status

Coverage increased (+0.02%) to 99.32% when pulling 093a996 on JasonYCHuang:i18n into 8db37b1 on shakacode:master.

@coveralls
Copy link

coveralls commented Jan 8, 2017

Coverage Status

Coverage increased (+0.02%) to 99.32% when pulling d08a057 on JasonYCHuang:i18n into 8db37b1 on shakacode:master.

@JasonYCHuang
Copy link
Contributor Author

Review status: 7 of 11 files reviewed at latest revision, 9 unresolved discussions.


docs/basics/i18n.md, line 7 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

and intl

??

update description.


docs/basics/i18n.md, line 32 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Could you only use the files that react-intl wants? and not use the Rails files?

Not sure I understand your questions.

react-intl wants locale files in json format, and I generate translations.js & default.js. Their sources are rails locale files.

I update the summary, and hope this will be more clear.


docs/basics/i18n.md, line 37 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Where do these 2 files come from?

These 2 files are generated by lib/react_on_rails/locales_to_js.rb in this PR.


docs/basics/i18n.md, line 41 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

We could do a quick check to see if the generated file is older than any of the locale files.

Done.


lib/react_on_rails/locales_to_js.rb, line 71 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

do we need "set" in the method name?

Done.


lib/react_on_rails/locales_to_js.rb, line 74 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

do we need "set" in the method name?

Done.


.byebug_history, line 1 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@JasonYCHuang Please remove this file.

Done.


Comments from Reviewable

@justin808
Copy link
Member

Please either merge or rebase to address the merge conflict.

@justin808
Copy link
Member

Reviewed 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


README.md, line 175 at r4 (raw file):

### I18n

You can enable the i18n functionality with [react-intl](https://github.com/yahoo/react-intl). ReactOnRails also converts traditional Rails locale files, `*.yml`, to required javascript files, `translations.js` & `default.js`, automatically.

I think we should consider mentioning that we support an optional automatic conversion of Rails *.yml files.


docs/basics/i18n.md, line 32 at r5 (raw file):

automatically after you finished the following settings.

finished settings is probably not what you're trying to say.

maybe you mean "configured the following settings"


lib/react_on_rails/locales_to_js.rb, line 21 at r5 (raw file):

                                          .select { |f| File.exist?(f) }
      files = files.sort_by { |f| File.mtime(f) }
      files.last(2).map { |f| File.basename(f) }

are there a small enough number of files that the performance would never matter?

Here's how we do it for the test helper: https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/test_helper/webpack_assets_status_checker.rb#L28

I actually think that's probably not super efficient either.


Comments from Reviewable

@justin808
Copy link
Member

Just a few comments remaining.

@JasonYCHuang JasonYCHuang force-pushed the i18n branch 2 times, most recently from d277795 to 24a4d79 Compare January 11, 2017 16:33
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.321% when pulling 24a4d79 on JasonYCHuang:i18n into a97e209 on shakacode:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 11, 2017

Coverage Status

Coverage increased (+0.02%) to 99.321% when pulling 24a4d79 on JasonYCHuang:i18n into a97e209 on shakacode:master.

@JasonYCHuang
Copy link
Contributor Author

Review status: 6 of 11 files reviewed at latest revision, 3 unresolved discussions.


README.md, line 175 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I think we should consider mentioning that we support an optional automatic conversion of Rails *.yml files.

Done.


docs/basics/i18n.md, line 32 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

automatically after you finished the following settings.

finished settings is probably not what you're trying to say.

maybe you mean "configured the following settings"

Done.


lib/react_on_rails/locales_to_js.rb, line 21 at r5 (raw file):

Previously, justin808 (Justin Gordon) wrote…

are there a small enough number of files that the performance would never matter?

Here's how we do it for the test helper: https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/test_helper/webpack_assets_status_checker.rb#L28

I actually think that's probably not super efficient either.

Try to improve this, and add test.


Comments from Reviewable

@justin808
Copy link
Member

Really super awesome!

See comments! so close!


Reviewed 5 of 5 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


CHANGELOG.md, line 11 at r6 (raw file):

- Removed foreman as a dependency. [#678](https://github.com/shakacode/react_on_rails/pull/678) by [x2es](https://github.com/x2es).

### Changed

Added...


spec/react_on_rails/locales_to_js_spec.rb, line 46 at r6 (raw file):

      it "doesn't update files" do
        mt = File.mtime(translations_path)
        sleep 1

Let's avoid the sleep!

http://stackoverflow.com/questions/32300446/stubbing-time-now-with-rspec

Set the time to be one minute ago.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Jan 12, 2017

Coverage Status

Coverage increased (+0.02%) to 99.321% when pulling c0b4715 on JasonYCHuang:i18n into a97e209 on shakacode:master.

@JasonYCHuang
Copy link
Contributor Author

@justin808 Thank you for patient reviews.
Hope we can finish the PR this week. 😄


Review status: 9 of 11 files reviewed at latest revision, 2 unresolved discussions.


CHANGELOG.md, line 11 at r6 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Added...

Done.


spec/react_on_rails/locales_to_js_spec.rb, line 46 at r6 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Let's avoid the sleep!

http://stackoverflow.com/questions/32300446/stubbing-time-now-with-rspec

Set the time to be one minute ago.

Done.


Comments from Reviewable

@justin808
Copy link
Member

:lgtm:

Awesome!

I'll ship later today.


Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@justin808 justin808 merged commit 9f0f63b into shakacode:master Jan 12, 2017
justin808 pushed a commit that referenced this pull request Feb 11, 2017
justin808 pushed a commit to shakacode/react-webpack-rails-tutorial that referenced this pull request Mar 2, 2017
# I18N OPTIONS
################################################################################
# Replace the following line to the location where you keep translation.js & default.js.
config.i18n_dir = Rails.root.join("client", "app", "libs", "i18n")
Copy link
Member

Choose a reason for hiding this comment

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

@JasonYCHuang We added this here, with an invalid directory, and later we abort deploying or testing when this directory does not exist.

ryanbelke pushed a commit to ryanbelke/rails-react-yourtime that referenced this pull request Jan 4, 2019
joseph0919 pushed a commit to joseph0919/React_Webpack_Rails_Tutorial that referenced this pull request May 3, 2019
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