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

Improve i18n Integration #748

Merged
merged 2 commits into from
Mar 5, 2017
Merged

Improve i18n Integration #748

merged 2 commits into from
Mar 5, 2017

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Mar 4, 2017

  • Better error message if the value of the i18n directory is invalid.
  • Added test for configuration error message.
  • DRY'd up code for checking if this config value is set.
  • Improved comments in sample react_on_rails.rb config file

This change is Reviewable

@justin808
Copy link
Member Author

@JasonYCHuang Please review!

* Better error message if the value of the i18n directory is invalid.
* Added test for configuration error message.
* DRY'd up code for checking if this config value is set.
* Improved comments in sample react_on_rails.rb config file
@justin808
Copy link
Member Author

@robwise any quick review?

@coveralls
Copy link

coveralls commented Mar 4, 2017

Coverage Status

Coverage increased (+0.004%) to 99.334% when pulling c6e6911 on tweaks-for-i18n into 6e3edd1 on master.

@justin808
Copy link
Member Author

@JasonYCHuang and @robwise Please review!

@JasonYCHuang
Copy link
Contributor

:lgtm:

I created a test project to test this, and it works good.

Maybe we can add red color to error message in the future.
That will be easier to identify what happens.


Review status: 0 of 8 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@justin808 justin808 merged commit ce31769 into master Mar 5, 2017
@justin808 justin808 deleted the tweaks-for-i18n branch March 5, 2017 09:52
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