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

Properly translate renamed resources #2044

Merged
merged 1 commit into from
Apr 3, 2013
Merged

Properly translate renamed resources #2044

merged 1 commit into from
Apr 3, 2013

Conversation

seanlinsley
Copy link
Contributor

This is for #1884, and is a re-wiring of #1885.

The initial problem that #1885 solved was that if you used the :as option, translation just didn't happen:

def resource_label
  if @options[:as]
    # just return the string
  else
    # actually do translation
  end
end

Unfortunately, to do so #1885 added a lot of duplicated code. This PR is an attempt to find a middle ground.

I had to create my own translate method, because for some reason String#human wasn't properly respecting i18n_key and the activerecord.models I18n scope.

But the result is that now you can rename a resource:

ActiveAdmin.register Foo, as: 'Bar'

and can have custom translations for Bar

en:
  activerecord:
    models:
      bar:
        one: "Baz"
        other: "A great deal of Bazs"

@seanlinsley
Copy link
Contributor Author

@gregbell since you're around now, any thoughts on this?

@gregbell
Copy link
Contributor

gregbell commented Apr 3, 2013

I think that a cohesive overhaul of the localization in Active Admin is needed... however, in the meantime, this seems like a great addition that solves a real problem. I like it.

My only comment that it's a bit awkward for non-activerecord i18n_keys to be placed in the "activerecord.models" namespace. But it's not the end of the world.

@seanlinsley
Copy link
Contributor Author

It's either that, or make up an i18n scope. If you're already translating your app, it seems to make sense to put pseudo-model translations in the same place as translations for real models.

@seanlinsley
Copy link
Contributor Author

Think we can include this in the release?

@gregbell
Copy link
Contributor

gregbell commented Apr 3, 2013

Yes, I am just running the tests on my machine. Assuming everything is all good, I'll merge it in and then release the new version.

@seanlinsley
Copy link
Contributor Author

I should note that there are likely to be a lot of features/bugfixes that aren't in the CHANGELOG

gregbell added a commit that referenced this pull request Apr 3, 2013
@gregbell gregbell merged commit 757ecf2 into activeadmin:master Apr 3, 2013
@seanlinsley seanlinsley deleted the bugfix/1884-translate-resource-name branch April 3, 2013 22:55
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.

2 participants