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

Support translations for ActiveRecord models #298

Merged
merged 1 commit into from
Dec 7, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

* [#251] [FEATURE] Raise a helpful error when an attribute is missing from
`ATTRIBUTE_TYPES`
* [#298] [FEATURE] Support ActiveRecord model I18n translations
* [#231] [UI] Fix layout issue on show page where a long label next to an empty
value would cause following fields on the page to be mis-aligned.
* [#259] [BUGFIX] Make installation generator more robust
Expand Down
12 changes: 12 additions & 0 deletions app/helpers/administrate/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,17 @@ def render_field(field, locals = {})
locals.merge!(field: field)
render locals: locals, partial: field.to_partial_path
end

def display_resource_name(resource_name)
resource_name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be resource?

Copy link

Choose a reason for hiding this comment

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

I would think that "resource" would refer to the whole model object, but that's just me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm siding with @mcmire on this. I've been using resource to refer to instances of the ActiveRecord models. Here, we're referring to the class names of the ActiveRecord models.

I think if we end up going with @mcmire's suggestion in #298 (comment), then we should rename this to resource_class or something.

to_s.
classify.
constantize.
model_name.
human(
count: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always 0? how do we handle plurals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoelQ this dictates how the resource names show up in the sidebar, and in the header on the index page. I think in both of those cases, we always want the displayed name to be in the plural form.

customers___administrate_prototype

I started to code out an argument to this function that would let you specify a custom count, but I think YAGNI applies to that. If we need to display the singular form in the future, we can adjust this method accordingly.

default: resource_name.to_s.pluralize.titleize,
)
end
end
end
2 changes: 1 addition & 1 deletion app/views/administrate/application/_sidebar.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ as defined by the DashboardManifest.
<% DashboardManifest::DASHBOARDS.each do |resource| %>
<li>
<%= link_to(
resource.to_s.titleize,
display_resource_name(resource),
Copy link

Choose a reason for hiding this comment

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

I honestly think that the mapping from a resource name to a model class shouldn't be inferred and should always be explicit. This would obviate the need to do classify and constantize above. I realize that doesn't have anything to do with this PR, but it would simplify the above logic at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcmire completely agreed. I'm planning to change the DashboardManifest::DASHBOARDS constant to an array of constants instead of an array of symbols. This would let us support namespaced models (#32, #179), and hopefully get rid of a lot of name inferences.

[Administrate::NAMESPACE, resource],
class: "sidebar__link sidebar__link--#{nav_link_state(resource)}"
) %>
Expand Down
4 changes: 3 additions & 1 deletion app/views/administrate/application/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ It renders the `_table` partial to display details about the resources.
[1]: http://www.rubydoc.info/gems/administrate/Administrate/Page/Table
%>

<% content_for(:title) { page.resource_name.pluralize.titleize } %>
<% content_for(:title) do %>
<%= display_resource_name(page.resource_name) %>
<% end %>

<% content_for(:search) do %>
<form class="search">
Expand Down
21 changes: 21 additions & 0 deletions spec/features/sidebar_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,25 @@

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

it "displays translated name of model" do
translations = {
activerecord: {
models: {
customer: {
one: "User",
other: "Users",
},
},
},
}

with_translations(:en, translations) do
visit admin_customers_path

sidebar = find(".sidebar__list")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this has double underscores?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're using BEM syntax for our CSS. In this case, the CSS class would read as: "The list element inside the sidebar block", or something like that.

CSS selector formats are something that we could have a whole other discussion on - I'm still not sure what the best solution is for Administrate. I chose BEM because many designers at thoughtbot have had pleasant experiences working with it, and it makes it a bit easier to write maintainable (S)CSS.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

expect(sidebar).to have_link("Users")
expect(page).to have_header("Users")
end
end
end
44 changes: 44 additions & 0 deletions spec/helpers/administrate/application_helper_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
require "rails_helper"

RSpec.describe Administrate::ApplicationHelper do
describe "#display_resource_name" do
it "defaults to the plural of the model name" do
displayed = display_resource_name(:customer)

expect(displayed).to eq("Customers")
end

it "handles string arguments" do
displayed = display_resource_name("customer")

expect(displayed).to eq("Customers")
end

it "handles plural arguments" do
displayed = display_resource_name(:customers)

expect(displayed).to eq("Customers")
end

context "when translations are defined" do
it "uses the plural of the defined translation" do
translations = {
activerecord: {
models: {
customer: {
one: "User",
other: "Users",
},
},
},
}

with_translations(:en, translations) do
displayed = display_resource_name(:customer)

expect(displayed).to eq("Users")
end
end
end
end
end
11 changes: 11 additions & 0 deletions spec/support/i18n.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
RSpec.configure do |config|
config.include AbstractController::Translation

def with_translations(locale, translations)
Copy link

Choose a reason for hiding this comment

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

Nice method here, love that it takes a block.

original_backend = I18n.backend

I18n.backend = I18n::Backend::KeyValue.new({}, true)
I18n.backend.store_translations(locale, translations)

yield
ensure
I18n.backend = original_backend
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

end