codeforamerica / ohana-api

The open source API directory of community social services.
http://ohana-api-demo.herokuapp.com/api
BSD 3-Clause "New" or "Revised" License
185 stars 344 forks source link

Expand the use of i18n #421

Closed md5 closed 7 years ago

md5 commented 7 years ago

This PR expands the use of i18n in the Ohana API admin. The following types of strings have been moved to the locale file:

For these strings, I have made heavy use of the Rails "lazy lookup" i18n mechanism, using dot-prefixed keys to access localizations that are scoped to the calling view. I also made use of a seldom-used "alias" feature of the i18n library by using a colon-prefixed value to share the value of one of the i18n strings in two partials.

In addition to the placeholder changes, I started to look at moving all field labels to i18n as well (using a combination of the *.activerecord.attributes and *.helpers.label scopes as appropriate). However, there are quite a few labels that are not using i18n and I thought I'd check to see if you're amenable to this change before I spent any more time on this.

The initial motivation for this PR was to rename "service areas" as "neighborhoods" in a possible Ohana deployment for the LA Promise Zone initiative. That isn't yet possible since I haven't dealt with the localization of Service#service_areas, but there is a clear path forward to making it possible.

In terms of customizing this for downstream Ohana deployments, that can either be done by modifying en.yml directly in the forked code base, or by adding an additional i18n file to avoid future merge conflicts (e.g. local-overrides.en.yml).

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 99.368% when pulling d1664790acd41496f1905fc8292f26d8e7a5cd85 on appropriate:expanded-i18n into fbdd8054493a1afec4e359c56de909b06c1e20c1 on codeforamerica:master.

md5 commented 7 years ago

@monfresh How do you feel about these changes? Do you think this is an avenue worth pursuing?

monfresh commented 7 years ago

Yes, this is great, thanks! Is this ready to merge?

md5 commented 7 years ago

@monfresh This could be merged as is, but the remaining changes I think should be made are to replace the per-field JS for Select2 with a single piece of JS that keys off the .select2 class and to use i18n for ActiveRecord models and attributes in more places to allow them to be customized. I can make the latter change as a new PR if you prefer, as well as the former change if you think it's a good idea.

Another change I was thinking could be nice would be to add an initializer that adds config/locales/overrides/*.yml to I18n.load_path and to add an empty config/locales/overrides/en.yml file. This would provide a standard place for forked projects to override Ohana's strings. It's possible to use something like config/locales/overrides.en.yml with the default load_path, but the handling of load_path is dependent on glob ordering to ensure that overrides.en.yml comes after en.yml. If you wanted to have (say) a Portuguese localization, then overrides.pt.yml would not actually override pt.yml, while a file from a later load_path entry would.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.0007%) to 99.367% when pulling c5ce59343f292257efeecfde9d362cdf2f1fbb51 on appropriate:expanded-i18n into fbdd8054493a1afec4e359c56de909b06c1e20c1 on codeforamerica:master.

md5 commented 7 years ago

@monfresh I've pushed a commit to update the attribute labels and helper labels for AR (7fdc0aa) as well as another commit to reduce the number of errors shown to users in certain cases (c5ce593).

In particular, when a field has a presence validation as well as other validations, I set those fields to have allow_blank: true for the other validations. This means that a blank field will only have a "can't be blank" error in these cases. The other validations only kick in when the value isn't blank.

The one place I didn't do this is on the date: true validation in HolidaySchedule. Doing so was making the specs fail in weird ways. I think I would personally ditch the DateValidator class and use validates_timeliness instead.

I'm also going to ignore the 6 "new" warnings from Code Climate since it's complaining about quoted string style for lines where I changed the contents of the string, not the quoting.

md5 commented 7 years ago

@monfresh Just wanted to bump you on this PR. No rush, but I'd like to merge this into ohana-api-la and I'd rather merge it from upstream if possible.

monfresh commented 7 years ago

I was out on vacation the past 10 days. I'll take a look at your PRs this week. Thanks for all the contributions!

monfresh commented 7 years ago

Thanks again for doing this. If you can please fix the Code Climate issues (quotes versus single quotes), this will be good to merge.

md5 commented 7 years ago

I fixed the Code Climate issues and rebased against master to avoid snyk vulnerability reports.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.0007%) to 99.367% when pulling 12db6a7ea7984d111646f02c4226e81bf681c287 on appropriate:expanded-i18n into 2aa8be105ccd034bf82512cb327ad0a31c483cfa on codeforamerica:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.0007%) to 99.367% when pulling 12db6a7ea7984d111646f02c4226e81bf681c287 on appropriate:expanded-i18n into 2aa8be105ccd034bf82512cb327ad0a31c483cfa on codeforamerica:master.