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

Fix default_url_options example in i18n guide #14564

Merged
merged 1 commit into from
Apr 3, 2014

Conversation

killthekitten
Copy link
Contributor

i18n guide has an example, which can break normal link_to workflow (somebody forgot about options).

Also, links in these quotes are broken, and I can't understand what was their target:

Rails contains infrastructure for "centralizing dynamic decisions about the URLs" in its [`ApplicationController#default_url_options`](http://api.rubyonrails.org/classes/ActionController/Base.html#M000515), which is useful precisely in this scenario: it enables us to set "defaults" for [`url_for`](http://api.rubyonrails.org/classes/ActionController/Base.html#M000503) and helper methods dependent on it (by implementing/overriding this method).

@rafaelfranca
Copy link
Member

@killthekitten
Copy link
Contributor Author

@rafaelfranca 👍 thank you for help, didn't have time to find this links by myself

@killthekitten
Copy link
Contributor Author

@rafaelfranca and I'm not sure is it OK to reverse merge options like I did in example?

I mean, should we keep this:

{ locale: I18n.locale }.merge options

or this:

options.merge{ locale: I18n.locale }

@rafaelfranca
Copy link
Member

The first is better. If you want to override in the call of url_for it should be possible.

So with first url_for(:locale => :us) would work. With the second it will always use the I18n.locale

rafaelfranca added a commit that referenced this pull request Apr 3, 2014
Fix default_url_options example in i18n guide
@rafaelfranca rafaelfranca merged commit 79f06a9 into rails:master Apr 3, 2014
@rafaelfranca
Copy link
Member

Thank you for the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants