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

Add translations for ActiveRecord models #180

Closed
wants to merge 1 commit into from
Closed

Add translations for ActiveRecord models #180

wants to merge 1 commit into from

Conversation

mgrachev
Copy link
Contributor

@mgrachev mgrachev commented Nov 6, 2015

Hi. Added translation support for ActiveRecord models in the sidebar and on the index page (header).

File locale must be of such content:

en:
  activerecord:
    models:
      bet: Rates

@c-lliope
Copy link
Contributor

Hey, @mgrachev! Sorry I haven't responded sooner.

Could you add some tests for this? Maybe something like this, in spec/features/sidebar_spec.rb?

it "honors model translations" do
  allow(I18n).to receive(:translate).with("customer").and_return("user")

  visit admin_customers_path

  sidebar = find(".sidebar__list")
  expect(sidebar).to have_link("Users")
  expect(page).to have_header("Users")
end

@@ -11,7 +11,7 @@ as defined by the DashboardManifest.
<% DashboardManifest::DASHBOARDS.each do |resource| %>
<li>
<%= link_to(
resource.to_s.titleize,
resource.to_s.classify.constantize.model_name.human(default: resource.to_s.titleize),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to pluralize this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The user decides how to display the model name through a file locale. In default pluralization.

@@ -8,4 +8,14 @@

expect(active_link.text).to eq "Customers"
end

it "displays translated name of model" do
allow(Customer.model_name).to receive(:human).and_return('Users')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@mgrachev
Copy link
Contributor Author

Added test

@mgrachev
Copy link
Contributor Author

mgrachev commented Dec 2, 2015

Please, restart the build on CircleCI

@c-lliope
Copy link
Contributor

c-lliope commented Dec 6, 2015

@mgrachev The build is failing because of a Nokogiri security vulnerability, which I've fixed in #285. If you rebase on master, the build should pass and I'll be able to merge it in.

Thanks!

@c-lliope
Copy link
Contributor

c-lliope commented Dec 6, 2015

@mgrachev I've cleaned it up a bit and added more thorough tests in #298. Let's move discussion over there.

Thanks for your work!

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