Uk/1562 add i18n js gem #1564

jeronimo · 2017-05-08T17:03:19Z

Task - #1562

Comments on steps:

Did not create precompiled separate files because:
- would have to leave default assets pipeline.
- with config.i18n.available_locales translations are not that big anymore.
- it might allow to change translations dynamically.
It falls back to default translation and doesn't break javascript. The problem is still with back-end translations inside placeholders for input fields if translation is still missing.

enricostano · 2017-05-10T08:34:59Z

Nice! 👏

daniellemoorhead · 2017-05-12T01:13:10Z

@enricostano you ok to review this before we push it over to Australia for final test/merge?

enricostano

👍

daniellemoorhead · 2017-05-19T01:29:26Z

Hey @oeoeaio I reckon this one is going to be a quick push through yourself and Sally to get it merged. Will leave you to it :)

RohanM

In this PR I see a lot of cases where we try implementing something one way and then switch to doing it another way in a later commit. To me that reads a bit like research / experimentation rather than building. That's a necessary and important part about developing something, but a side-effect is that any developer looking back on this feature will have to understand all the side investigations to understand what's going on in the feature. We'd like to make it as easy as possible for other people to look over this code and understand what's going on.

To achieve that, sometimes in this situation I'd recommend editing the git history to clean up a few things. But in this case there are several changes happening in each commit, which makes it harder to work with. Therefore, it seems like it'd be quicker to treat this branch as a spike and re-implement it fresh in another branch. With the knowledge developed in this one I expect it would fall out quite quickly.

Thanks for all your work on this one, and sorry to add more to your plate. Let me know if you have any questions!

RohanM · 2017-05-24T05:48:15Z

app/assets/javascripts/darkswarm/i18n.translate.js.coffee

-    text = text.split("%{#{name}}").join(value)
-  text
+  I18n.t(key, options)
+  # dict = I18n


Committed comments later removed.

RohanM · 2017-05-24T05:48:44Z

app/models/spree/variant_decorator.rb

@@ -9,6 +9,7 @@
  remove_method :options_text if instance_methods(false).include? :options_text
  include OpenFoodNetwork::VariantAndLineItemNaming

+  acts_as_taggable


Does this belong in this commit (or even this PR)? It seems like it might be a separate concern.

RohanM · 2017-05-24T05:49:53Z

app/models/spree/variant_decorator.rb

@@ -9,7 +9,6 @@
  remove_method :options_text if instance_methods(false).include? :options_text
  include OpenFoodNetwork::VariantAndLineItemNaming

-  acts_as_taggable


Aha, I see that this has been removed. In this sort of case I'd recommend editing history to remove the introduction in the first place. But see above for comments about treating this as a spike.

RohanM · 2017-05-24T05:51:27Z

config/application.rb

@@ -108,7 +109,5 @@ class Application < Rails::Application
    config.assets.precompile += ['qz/*']

    config.active_support.escape_html_entities_in_json = true
-
-    config.middleware.use I18n::JS::Middleware


This line was added and then removed - I take it that we don't need it? In this sort of case I'd recommend editing history to remove the introduction in the first place. But see above for comments about treating this as a spike.

RohanM · 2017-05-24T05:52:32Z

config/i18n-js.yml

@@ -1,35 +0,0 @@
-# Split context in several files.


This change looks like another case of trying one approach in one commit, then switching to another in a later commit. See above for comments about treating this PR as a spike.

RohanM · 2017-05-24T05:55:50Z

spec/javascripts/application_spec.js

@@ -13,6 +13,7 @@
 //= require textAngular-sanitize.min.js
 //= require textAngular.min.js
 //= require moment
+//= require i18n


Is this change related to Karma? If not, I'd suggest moving this change to a separate commit. But see above for comments about treating this as a spike.

jeronimo · 2017-05-24T10:45:50Z

@RohanM, You are totally right it felt like a spike :) At some point it felt right to leave it with that commit history as it would help to understand what is necessary to use from gem's documentation. But apparently not. Fair enough :)
Instead of creating another branch it was easier to squash current one and the PR stays the same.
Old commits' history is here https://github.com/jeronimo/openfoodnetwork/commits/uk/add-i18n-js-gem-spike

lin-d-hop · 2017-05-30T14:47:49Z

@RohanM Are you happy to sign this off now ready for merge at next opportunity?

RohanM · 2017-05-31T05:52:25Z

Hi @jeronimo,

Unfortunately, squashing the commits puts the entire 22 file, 150 line diff in a single commit - that's too much code for someone to easily absorb, especially when that one commit is accomplishing many distinct things. Having the code in one commit obscures the steps that went into achieving the feature. Additionally, in the future we might discover a problem that involves this code. If it's in a single commit, the we're not able to use git bisect to narrow down on the cause of the problem, and it's harder for a developer to understand the code.

Because of those reasons, it's useful to break the code down into smaller commits (both for a reviewer and for any future developers who might work with this code). Preferably each commit introduces one part of the change, comes with its own specs, and those specs pass. That way a developer can read the commit message and glance at the code and confirm that the code does indeed do what the commit message says. And because the specs build green at most if not all commits along the way, we can bisect over it and get useful results.

Finally, a comment on the situation where a branch is broken down into multiple commits but changes one thing and then changes it back later. Although this can be useful in describing the process taken to arrive at a feature, it can result in unnecessary conflicts when rebasing. And when multiple changes back and fourth are combined in a single commit, it can be hard to pick apart what's happening. So I'd see this situation as a trade-off, and unless I really wanted to illustrate a path that didn't work out, I'd tend to the side of simply removing those changes from the code, and writing about it in the PR description (which a future developer could look up from the merge commit).

For those reasons, would you be able to rewrite this PR step by step?

jeronimo · 2017-05-31T21:46:42Z

@RohanM, actually I do not agree. Nearly 60% of added code is test coverage. 20% of code is a workaround until Spree gets upgrade. The rest of the code is used from Rails guides and READMEs and this whole commit is like a small feature, which is great for git bisect. If you would notice, original PR had 3 commits only, and that was only because I tried various approaches how to use i18n-js gem until I chose best one (IMHO) for our case.

But, if you still do think it is necessary to split, maybe you could give an example with this code for the future reference we could follow?

mkllnk

Hi @jeronimo. I agree with Rohan that smaller commits are easier to read. Your commit is not huge, but it could be nicer. Below I commented on a few things that could be separated, in my opinion. Ideally, you read one commit at a time like pages of a book. And that includes test cases.

Anyway, that's great work. I have a question. We include all supported translations in all.js, right? Can you tell how big each locale is? How much longer does it need to load with e.g. five locales?
I had in mind that each locale might be available as separate js file (except the default locale). Each user wouldn't need to load all the unused locales then. I'm just imagining a platform that wants to support a lot of languages.

mkllnk · 2017-06-08T01:04:52Z

Gemfile

@@ -4,6 +4,7 @@ ruby "2.1.5"
 gem 'rails', '3.2.21'
 gem 'rails-i18n', '~> 3.0.0'
 gem 'i18n', '~> 0.6.11'
+gem 'i18n-js', '~> 3.0.0'


Adding a gem can be in a single commit.

mkllnk · 2017-06-08T01:06:55Z

config/application.rb

@@ -71,6 +71,7 @@ class Application < Rails::Application
    # The default locale is :en and all translations from config/locales/*.rb,yml are auto loaded.
    # config.i18n.load_path += Dir[Rails.root.join('my', 'locales', '*.{rb,yml}').to_s]
    config.i18n.default_locale = ENV["LOCALE"]
+    config.i18n.available_locales = ENV["AVAILABLE_LOCALES"].andand.split(',').andand.map(&:strip) || [config.i18n.default_locale]


Support for multiple locales can be seen as a feature on it's own. It can be in a separate commit.

jeronimo · 2017-06-08T21:38:58Z

@RohanM, @mkllnk, @oeoeaio please have a look now :)

mkllnk

Wow. This is much easier to read. Excellent.

mkllnk · 2017-06-09T04:54:56Z

config/initializers/i18n-js.rb

+#   no: "No"
+#   yes: "Yes"
+# Which become to true and false and those have no #to_sym method
+# TODO - remove this after spree core locales are fixed


Great. This way it will be super easy to see what needs to be changed after the Spree upgrade.

mkllnk · 2017-06-09T06:09:33Z

I think before we merge this, we need to update our deploy scripts. @jeronimo Have a look at: https://github.com/openfoodfoundation/ofn-install/blob/master/roles/app/templates/post-receive.j2

We need to add the line:

$BUNDLE exec rake tmp:cache:clear

Can that happen any time or do we need to do it before the asset compilation?

jeronimo · 2017-06-12T21:16:00Z

@mkllnk It seems it has to be before assets:precompile. More details inside PR openfoodfoundation/ofn-install#73.

Note: tmp:cache:clear need should disappear after upgrade to rails 4.x which will introduce sprockets.

mkllnk

Great. This is ready for testing now.

This makes sure there is a value in `config.i18n.available_locales` so that i18n-js doesn't crash.

mkllnk · 2017-08-02T01:54:14Z

@sstead This adds a feature to provide more than one language. Currently, our staging server is configured to show only English. Nothing should change. And we don't have a UI element for language switching yet. That comes later.

sstead · 2017-08-02T02:16:35Z

Looks fine.

mkllnk · 2017-08-02T03:01:03Z

Great work @jeronimo.

I needed to add a fall-back strategy for old application.yml files that don't have the locale set correctly. Otherwise that i18n-js gem would crash. But that wasn't your fault. Glad to have this merged.

daniellemoorhead added this to the Next small release (1.8.11 or 1.9.1) milestone May 12, 2017

daniellemoorhead added the uk label May 12, 2017

daniellemoorhead assigned jeronimo May 12, 2017

enricostano approved these changes May 12, 2017

View reviewed changes

jeronimo mentioned this pull request May 12, 2017

Uk/1563 backend for persistent lang switching #1572

Merged

daniellemoorhead requested a review from oeoeaio May 19, 2017 01:28

daniellemoorhead modified the milestones: June dot point release, May dot point release May 24, 2017

RohanM reviewed May 24, 2017

View reviewed changes

mkllnk reviewed Jun 8, 2017

View reviewed changes

mkllnk approved these changes Jun 9, 2017

View reviewed changes

jeronimo mentioned this pull request Jun 12, 2017

Running tmp:cache:clear for i18n-js before assets:precompile openfoodfoundation/ofn-install#73

Merged

daniellemoorhead modified the milestones: July dot point release, June dot point release Jun 30, 2017

mkllnk approved these changes Jul 26, 2017

View reviewed changes

mkllnk modified the milestones: August dot point release, July dot point release Jul 28, 2017

jeronimo added 3 commits August 2, 2017 09:57

Add i18n-js gem

3adb13e

Add temporary i18n-js workaround for Spree translations until upgrade

d2ba465

Require new i18n-js translations in darkswarm and admin

2637574

jeronimo and others added 8 commits August 2, 2017 09:57

Render i18n-js config inside darkswarm and admin layouts

f396d30

Add i18n helper for controllers to switch language by params

9429695

Add available_locales config feature

5166a3d

Adapt karma js tests for i18n-js

c6f6f5b

Add test coverage for switching language in darkswarm and admin

ec6a365

Update README

e4627fe

Correct typo in comment

199bfe5

Add legacy and default values for LOCALE

686a8f3

This makes sure there is a value in `config.i18n.available_locales` so that i18n-js doesn't crash.

RohanM merged commit 686a8f3 into openfoodfoundation:master Aug 2, 2017

mkllnk requested a deployment to staging August 2, 2017 03:08 Abandoned

mkllnk mentioned this pull request Aug 11, 2017

Missing translations in all.js #1761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uk/1562 add i18n js gem #1564

Uk/1562 add i18n js gem #1564

jeronimo commented May 8, 2017

enricostano commented May 10, 2017

daniellemoorhead commented May 12, 2017

enricostano left a comment

daniellemoorhead commented May 19, 2017

RohanM left a comment

RohanM May 24, 2017

RohanM May 24, 2017

RohanM May 24, 2017

RohanM May 24, 2017

RohanM May 24, 2017

RohanM May 24, 2017

jeronimo commented May 24, 2017

lin-d-hop commented May 30, 2017

RohanM commented May 31, 2017

jeronimo commented May 31, 2017

mkllnk left a comment

mkllnk Jun 8, 2017

mkllnk Jun 8, 2017

jeronimo commented Jun 8, 2017

mkllnk left a comment

mkllnk Jun 9, 2017

mkllnk commented Jun 9, 2017

jeronimo commented Jun 12, 2017

mkllnk left a comment

mkllnk commented Aug 2, 2017

sstead commented Aug 2, 2017

mkllnk commented Aug 2, 2017

Uk/1562 add i18n js gem #1564

Uk/1562 add i18n js gem #1564

Conversation

jeronimo commented May 8, 2017

enricostano commented May 10, 2017

daniellemoorhead commented May 12, 2017

enricostano left a comment

Choose a reason for hiding this comment

daniellemoorhead commented May 19, 2017

RohanM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeronimo commented May 24, 2017

lin-d-hop commented May 30, 2017

RohanM commented May 31, 2017

jeronimo commented May 31, 2017

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeronimo commented Jun 8, 2017

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk commented Jun 9, 2017

jeronimo commented Jun 12, 2017

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk commented Aug 2, 2017

sstead commented Aug 2, 2017

mkllnk commented Aug 2, 2017