Fix for ticket 1000: support changing locale for EnumString translations #34

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

@iox
Contributor
iox commented Feb 28, 2012

Hi,

I wrote a fix for https://hobo.lighthouseapp.com/projects/8324/tickets/1000-i18n-support-switching-locales-with-enum_string. Basically I modify the view, input and editor for EnumString fields so they support i18n. I also believe we could get rid of some enumstring i18n code in hobo_fields, if you want I can take a look at that too.

I'm not sure if this is the right branch to make a pull request. Maybe you prefer me to test this against Hobo 1.4 and pull the request there. Just tell me :).

@bryanlarsen
Collaborator

Thank you very much!

1-3-stable was probably the correct branch. 1.4 still has a couple of major bugs making it difficult to test with. Editors in particular have been completely rewritten in 1.4, but the way your editor calls out to string-select-editor means that it should work without any fixes in 1.4. Good show!

I'd like to get a sign off from Domizio on this before pushing it. I don't use i18n myself, so he's really the guy that should be making this call.

And yes, I would like a pull request removing the old i18n enum_string code from hobo_fields. But I think we're better off just applying that to 1.4 since it has a greater chance of breaking somebody's code for less benefit.

@bryanlarsen
Collaborator

Here's Domizio's response:

"I don't have time to test it, but it seems OK. The only thing I don't like is the repetition of the same code in 3 different places. We should definitely use an helper!:

Maybe you can get somebody else on the list to give it a quick smoke test before merging into 1-3-stable. I have no qualms merging into 1.4 though.

The helper is a good idea. /hobo/lib/hobo/rapid/helper.rb would be the logical place to add the helper.

Also, the fact that you're doing a search for the column name is a red flag. Did you try using 'this_field'?

@iox
Contributor
iox commented Mar 1, 2012

Sorry about the repeated response: I was using the company user instead of personal :S. I'll repeat the answer:

Thanks a lot for the advice!

Domizio, you are completely right: using this_field makes it much cleaner. I just changed the code and it works perfectly.

Bryan, about using a helper, I need some help: the code looks similar but the result is different in the three occasions:

  • In the view I only translate one of the options.
  • In the editor I need: {:hotel=>"Hotel", :office=>"Office", :airport=>"Airport"}
  • In the input I need: [["Hotel", "hotel"], ["Office", "office"], ["Airport", "airport"]]

I just made another commit and it looks like it got attached to this pull request :).

@bryanlarsen
Collaborator

Now that you've gotten rid of the loop, I suspect that enough of your common code is gone to make a helper not worth while.

@bryanlarsen
Collaborator

iox, I just noticed that you totally nuked the labels parameter for the input. You certainly can and should add a comment to the documentation saying that activerecord.attributes.<model>.<field>s should be preferably used, but for backwards compatibility, the labels should still be honoured. Their is no need to attempt to translate any user supplied labels.

@iox
Contributor
iox commented Mar 15, 2012

Ok, you are right :). I'll take a look at it and update the pull request. Thanks!

@bryanlarsen
Collaborator

OK, this should be fixed in 83bb000 so this ticket can be considered closed for 1.4. If you want to put together a clean pull request for 1.3 I will apply it. I'm comfortable leaving this as 1.4 only, though.

@iox
Contributor
iox commented Apr 22, 2012

All right, I read your fix for 1.4 and tested it with 1.3. It works perfectly. Do you need me to make a new pull request? I'm not sure what a 'clean' pull request means.

@iox
Contributor
iox commented Jul 19, 2012

I'm going to close this issue: I believe this is not critical for 1.3 and it's working nicely in 1.4.

@iox iox closed this Jul 19, 2012
@pavlovich pavlovich referenced this pull request in pavlovich/hobo Jun 25, 2013
@iox iox Closes #34. Use bundle update in wizard in order to correctly resolve…
… jquery_rails dependency
5fa8249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment