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

New features #4

Merged
merged 6 commits into from
Apr 2, 2013
Merged

New features #4

merged 6 commits into from
Apr 2, 2013

Conversation

mathieugagne
Copy link

Hey Mateo,

I stumbled on your project yesterday trying to find a better way to integrate locales. Found it looked better than the other options out there plus you were already making use of http_accept_language!

I added a few things to it, let me know what you think. All tests are passing. It's quite a lot of changes in a single pull request, sorry about that.

Mainly the changes are:

  • Add a current_locale method
  • Automatically adds a before_filter for set_locale on ActionController (couldn't find a reason not to have it. Plus you get a localized version of other controllers that do not inherit from ApplicationController, like Devise would?)
  • Bundled it in a Railtie

Upcoming:

  • Generator for config file
  • link_to_locale helper method to change locales

I edited your Gemfile and gemspec and removed all your versioning thing in there. Looked quite deprecated to me, sorry if it wasn't the case.

@mateomurphy
Copy link
Owner

Hi Mathieu!

Most of this looks good. However the gemfile/gemspec changes break jeweller's workflow, so I can't merge those directly, can you remove them? Also, I'd rather not commit .rvmrc...

@mateomurphy
Copy link
Owner

Ok, I manually merged some of your changes, as I couldn't automatically do it due to the gemspec issue.

Regarding the view helper methods; they're too opinionated in the html they generate, and they wouldn't fit into any of the projects I'm using this gem in, so are a poor fit for something that is included automatically. Feel free to suggest some more generic alternatives though.

Thanks for your work!

@mathieugagne
Copy link
Author

I understand for the helper methods. Simply the way the locale is translated wouldn't fit. Maybe I would have left current_locale though, it's more verbose in the view but I don't mind.

I would have appreciated making the change myself though. I don't have any credit for the features now. Not that I care much, but it feels nice to have a merge request instead of closed request ;)

I'll add the install generator soon for the config.

@mateomurphy mateomurphy reopened this Apr 2, 2013
mateomurphy added a commit that referenced this pull request Apr 2, 2013
@mateomurphy mateomurphy merged commit 6489e74 into mateomurphy:master Apr 2, 2013
@mateomurphy
Copy link
Owner

Ok, I rolled back my changes, merged yours, and then merged mine back on top. I did end up changing everything quite heavily over the weekend though, so check the changes before creating the generator.

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